From 64a73b1419953bc4fbae5c1a339ad036468d54ab Mon Sep 17 00:00:00 2001 From: Goutham Pacha Ravi Date: Tue, 23 Aug 2016 15:33:21 -0400 Subject: [PATCH] Refactor Access Rules APIs - Pull up policy check to beginning of the APIs. - Avoid making access rules changes when one or more instances of the share are in an invalid state. - Add back the per rule share instance access status. This restoration provides better visibility for which rules were applied successfully. - Remove 'updating' and 'updating_multiple' as valid states for the share instance access rules status. - Deprecate the access rule state 'new' in favor of 'queued_to_apply' and the share instance access rules status 'out_of_sync' in favor of 'syncing'. In a new API micro-version: - Allow access rule changes irrespective of the share's access_rules_status. - Expose new access rule states and share's access_rules_status values. Access rules for each share instance now transition from 'queued_to_apply' to 'applying' to 'active' or 'error'; and from 'active', 'queued_to_apply', 'applying' or 'error' to 'queued_to_deny' to 'denying' to 'deleted'. APIImpact DocImpact Partially-implements: bp fix-and-improve-access-rules Co-Authored-By: Mike Rooney Change-Id: Ic25e63215b5ba723cbc8cab7c51789c698e76f28 --- api-ref/source/parameters.yaml | 21 +- api-ref/source/share-actions.inc | 68 +- manila/api/openstack/api_version_request.py | 8 +- .../openstack/rest_api_version_history.rst | 9 + manila/api/v1/shares.py | 26 +- manila/api/v2/shares.py | 12 +- manila/api/views/share_accesses.py | 15 + manila/api/views/shares.py | 8 + manila/common/constants.py | 28 +- manila/data/helper.py | 41 +- manila/db/api.py | 74 +- ..._add_share_instance_access_rules_status.py | 15 +- ...restore_share_instance_access_map_state.py | 99 ++ manila/db/sqlalchemy/api.py | 148 +-- manila/db/sqlalchemy/models.py | 57 +- manila/share/access.py | 630 ++++++++---- manila/share/api.py | 141 +-- manila/share/driver.py | 73 +- manila/share/drivers/cephfs/cephfs_native.py | 2 +- manila/share/drivers/huawei/v3/connection.py | 8 +- .../netapp/dataontap/cluster_mode/lib_base.py | 2 +- manila/share/drivers/zfsonlinux/driver.py | 2 +- manila/share/manager.py | 144 ++- manila/share/migration.py | 46 +- manila/share/rpcapi.py | 31 +- manila/tests/api/contrib/stubs.py | 3 + manila/tests/api/v1/test_shares.py | 31 + manila/tests/api/v2/test_shares.py | 140 ++- manila/tests/api/views/test_share_accesses.py | 11 + manila/tests/data/test_helper.py | 59 +- .../alembic/migrations_data_checks.py | 136 +++ manila/tests/db/sqlalchemy/test_api.py | 135 ++- manila/tests/db/sqlalchemy/test_models.py | 43 +- manila/tests/db_utils.py | 13 +- manila/tests/fake_share.py | 2 +- .../drivers/cephfs/test_cephfs_native.py | 30 +- .../share/drivers/huawei/test_huawei_nas.py | 15 +- .../dataontap/cluster_mode/test_lib_base.py | 4 +- .../share/drivers/zfsonlinux/test_driver.py | 15 +- manila/tests/share/test_access.py | 969 +++++++++++++----- manila/tests/share/test_api.py | 254 ++--- manila/tests/share/test_manager.py | 235 ++--- manila/tests/share/test_migration.py | 47 +- manila/tests/share/test_rpcapi.py | 25 +- manila/tests/test_utils.py | 20 +- manila/utils.py | 3 +- manila_tempest_tests/config.py | 2 +- ...ce-access-rule-state-7c08a91373b21557.yaml | 24 + 48 files changed, 2595 insertions(+), 1329 deletions(-) create mode 100644 manila/db/migrations/alembic/versions/54667b9cade7_restore_share_instance_access_map_state.py create mode 100644 releasenotes/notes/bug-1626249-reintroduce-per-share-instance-access-rule-state-7c08a91373b21557.yaml diff --git a/api-ref/source/parameters.yaml b/api-ref/source/parameters.yaml index abdc35c713..f088117ecf 100644 --- a/api-ref/source/parameters.yaml +++ b/api-ref/source/parameters.yaml @@ -506,19 +506,13 @@ access_rule_updated_at: type: string access_rules_status: description: | - The share instance access rules status. Valid value are ``active``, - ``error``, and ``out_of_sync``. + The share instance access rules status. A valid value is ``active``, + ``error``, or ``syncing``. In versions prior to 2.28, ``syncing`` was + represented with status ``out_of_sync``. in: body required: true type: string min_version: 2.10 -access_rules_status_1: - description: | - (Since API v2.10) The share access rules status. - Valid values are ``active``, ``error``, and ``out_of_sync``. - in: body - required: true - type: string access_share_id: description: | The UUID of the share to which you are granted @@ -3088,8 +3082,13 @@ source_cgsnapshot_member_id: type: string state: description: | - The access rule state. A valid value is - ``active`` or ``error``. + Prior to versions 2.28, the state of all access rules of a given share + is the same at all times. This could be ``new``, ``active`` or + ``error``. Since 2.28, the state of each access rule of a share is + independent of the others and can be ``queued_to_apply``, + ``applying``, ``active``, ``error``, ``queued_to_deny`` or ``denying``. + A new rule starts out in ``queued_to_apply`` state and is successfully + applied if it transitions to ``active`` state. in: body required: true type: string diff --git a/api-ref/source/share-actions.inc b/api-ref/source/share-actions.inc index 3aa996a58e..643befef6f 100644 --- a/api-ref/source/share-actions.inc +++ b/api-ref/source/share-actions.inc @@ -4,35 +4,9 @@ Share actions ============= -Grants or revokes share access, lists the permissions for a share, -and explicitly updates the state of a share. - -To grant or revoke share access, specify one of these supported -share access levels: - -- ``rw``. Read and write (RW) access. - -- ``ro``. Read-only (RO) access. - -You must also specify one of these supported authentication -methods: - -- ``ip``. Authenticates an instance through its IP address. A valid - format is ``XX.XX.XX.XX`` or ``XX.XX.XX.XX/XX``. For example - ``0.0.0.0/0``. - -- ``cert``. Authenticates an instance through a TLS certificate. - Specify the TLS identity as the IDENTKEY. A valid value is any - string up to 64 characters long in the common name (CN) of the - certificate. The meaning of a string depends on its - interpretation. - -- ``user``. Authenticates by a user or group name. A valid value is - an alphanumeric string that can contain some special characters - and is from 4 to 32 characters long. - -To verify that the access rules (ACL) were configured correctly for -a share, you list permissions for a share. +Share actions include granting or revoking share access, listing the +available access rules for a share, explicitly updating the state of a +share, resizing a share and un-managing a share. As administrator, you can reset the state of a share and force- delete a share in any state. Use the ``policy.json`` file to grant @@ -61,11 +35,38 @@ access_list": null} is valid for v1.0-2.6 Grant access ============ +All manila shares begin with no access. Clients must be provided with +explicit access via this API. + +To grant access, specify one of these supported share access levels: + +- ``rw``. Read and write (RW) access. + +- ``ro``. Read-only (RO) access. + +You must also specify one of these supported authentication +methods: + +- ``ip``. Authenticates an instance through its IP address. A valid + format is ``XX.XX.XX.XX`` or ``XX.XX.XX.XX/XX``. For example + ``0.0.0.0/0``. + +- ``cert``. Authenticates an instance through a TLS certificate. + Specify the TLS identity as the IDENTKEY. A valid value is any + string up to 64 characters long in the common name (CN) of the + certificate. The meaning of a string depends on its + interpretation. + +- ``user``. Authenticates by a user or group name. A valid value is + an alphanumeric string that can contain some special characters + and is from 4 to 32 characters long. + .. rest_method:: POST /v2/{tenant_id}/shares/{share_id}/action Grants access to a share. Normal response codes: 202 + Error response codes: badRequest(400), unauthorized(401), forbidden(403), itemNotFound(404) @@ -114,9 +115,12 @@ Revoke access .. rest_method:: POST /v2/{tenant_id}/shares/{share_id}/action -Revokes access from a share. +The shared file systems service stores each access rule in its database and +assigns it a unique ID. This ID can be used to revoke access after access +has been requested. Normal response codes: 202 + Error response codes: badRequest(400), unauthorized(401), forbidden(403), itemNotFound(404) @@ -142,9 +146,11 @@ List access rules .. rest_method:: POST /v2/{tenant_id}/shares/{share_id}/action -Lists access rules for a share. +Lists access rules for a share. The Access ID returned is necessary to deny +access. Normal response codes: 200 + Error response codes: badRequest(400), unauthorized(401), forbidden(403), itemNotFound(404) diff --git a/manila/api/openstack/api_version_request.py b/manila/api/openstack/api_version_request.py index a3b2d95279..7b31563fdd 100644 --- a/manila/api/openstack/api_version_request.py +++ b/manila/api/openstack/api_version_request.py @@ -84,15 +84,19 @@ REST_API_VERSION_HISTORY = """ spec. Also made the 'snapshot_support' extra spec optional. * 2.25 - Added quota-show detail API. * 2.26 - Removed 'nova_net_id' parameter from share_network API. - * 2.27 - Added share revert to snapshot API. + * 2.28 - Added transitional states to access rules and replaced all + transitional access_rules_status values of + shares (share_instances) with 'syncing'. Share action API + 'access_allow' now accepts rules even when a share or any of + its instances may have an access_rules_status set to 'error'. """ # The minimum and maximum versions of the API supported # The default api version request is defined to be the # minimum version of the API supported. _MIN_API_VERSION = "2.0" -_MAX_API_VERSION = "2.27" +_MAX_API_VERSION = "2.28" 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 ab5b8d1df3..1de3a3a14e 100644 --- a/manila/api/openstack/rest_api_version_history.rst +++ b/manila/api/openstack/rest_api_version_history.rst @@ -163,3 +163,12 @@ user documentation. snapshot. The share is reverted in place, and the snapshot must be the most recent one known to manila. The feature is controlled by a new standard optional extra spec, revert_to_snapshot_support. + +2.28 +---- + Added transitional states ('queued_to_apply' - was previously 'new', + 'queued_to_deny', 'applying' and 'denying') to access rules. + 'updating', 'updating_multiple' and 'out_of_sync' are no longer valid + values for the 'access_rules_status' field of shares, they have + been collapsed into the transitional state 'syncing'. Access rule changes + can be made independent of a share's 'access_rules_status'. diff --git a/manila/api/v1/shares.py b/manila/api/v1/shares.py index c1ef1cf23e..e0029c2785 100644 --- a/manila/api/v1/shares.py +++ b/manila/api/v1/shares.py @@ -30,6 +30,7 @@ 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.common import constants from manila import db from manila import exception from manila.i18n import _, _LI @@ -401,12 +402,34 @@ class ShareMixin(object): raise webob.exc.HTTPBadRequest(explanation=_( 'Ceph IDs may not contain periods')) - def _allow_access(self, req, id, body, enable_ceph=False): + @staticmethod + def _any_instance_has_errored_rules(share): + for instance in share['instances']: + access_rules_status = instance['access_rules_status'] + if access_rules_status == constants.SHARE_INSTANCE_RULES_ERROR: + return True + return False + + @wsgi.Controller.authorize('allow_access') + def _allow_access(self, req, id, body, enable_ceph=False, + allow_on_error_status=False): """Add share access rule.""" context = req.environ['manila.context'] access_data = body.get('allow_access', body.get('os-allow_access')) share = self.share_api.get(context, id) + if (not allow_on_error_status and + self._any_instance_has_errored_rules(share)): + msg = _("Access rules cannot be added while the share or any of " + "its replicas or migration copies has its " + "access_rules_status set to %(instance_rules_status)s. " + "Deny any rules in %(rule_state) state and try " + "again.") % { + 'instance_rules_status': constants.SHARE_INSTANCE_RULES_ERROR, + 'rule_state': constants.ACCESS_STATE_ERROR, + } + raise webob.exc.HTTPBadRequest(explanation=msg) + access_type = access_data['access_type'] access_to = access_data['access_to'] if access_type == 'ip': @@ -435,6 +458,7 @@ class ShareMixin(object): return self._access_view_builder.view(req, access) + @wsgi.Controller.authorize('deny_access') def _deny_access(self, req, id, body): """Remove share access rule.""" context = req.environ['manila.context'] diff --git a/manila/api/v2/shares.py b/manila/api/v2/shares.py index 3b78b5ecdc..80ee2ce109 100644 --- a/manila/api/v2/shares.py +++ b/manila/api/v2/shares.py @@ -323,10 +323,14 @@ class ShareController(shares.ShareMixin, @wsgi.action('allow_access') def allow_access(self, req, id, body): """Add share access rule.""" - if req.api_version_request < api_version.APIVersionRequest("2.13"): - return self._allow_access(req, id, body) - else: - return self._allow_access(req, id, body, enable_ceph=True) + args = (req, id, body) + kwargs = {} + if req.api_version_request >= api_version.APIVersionRequest("2.13"): + kwargs['enable_ceph'] = True + if req.api_version_request >= api_version.APIVersionRequest("2.28"): + kwargs['allow_on_error_status'] = True + + return self._allow_access(*args, **kwargs) @wsgi.Controller.api_version('2.0', '2.6') @wsgi.action('os-deny_access') diff --git a/manila/api/views/share_accesses.py b/manila/api/views/share_accesses.py index b51491e982..7847fa158b 100644 --- a/manila/api/views/share_accesses.py +++ b/manila/api/views/share_accesses.py @@ -14,6 +14,8 @@ # under the License. from manila.api import common +from manila.common import constants +from manila.share import api as share_api class ViewBuilder(common.ViewBuilder): @@ -22,6 +24,7 @@ class ViewBuilder(common.ViewBuilder): _collection_name = 'share_accesses' _detail_version_modifiers = [ "add_access_key", + "translate_transitional_statuses", ] def list_view(self, request, accesses): @@ -59,3 +62,15 @@ class ViewBuilder(common.ViewBuilder): @common.ViewBuilder.versioned_method("2.21") def add_access_key(self, context, access_dict, access): access_dict['access_key'] = access.get('access_key') + + @common.ViewBuilder.versioned_method("1.0", "2.27") + def translate_transitional_statuses(self, context, access_dict, access): + """In 2.28, the per access rule status was (re)introduced.""" + api = share_api.API() + share = api.get(context, access['share_id']) + + if (share['access_rules_status'] == + constants.SHARE_INSTANCE_RULES_SYNCING): + access_dict['state'] = constants.STATUS_NEW + else: + access_dict['state'] = share['access_rules_status'] diff --git a/manila/api/views/shares.py b/manila/api/views/shares.py index f592eeebaf..b224f452f0 100644 --- a/manila/api/views/shares.py +++ b/manila/api/views/shares.py @@ -14,6 +14,7 @@ # under the License. from manila.api import common +from manila.common import constants class ViewBuilder(common.ViewBuilder): @@ -31,6 +32,7 @@ class ViewBuilder(common.ViewBuilder): "add_user_id", "add_create_share_from_snapshot_support_field", "add_revert_to_snapshot_support_field", + "translate_access_rules_status", ] def summary_list(self, request, shares): @@ -154,6 +156,12 @@ class ViewBuilder(common.ViewBuilder): share_dict['revert_to_snapshot_support'] = share.get( 'revert_to_snapshot_support') + @common.ViewBuilder.versioned_method("2.10", "2.27") + def translate_access_rules_status(self, context, share_dict, share): + if (share['access_rules_status'] == + constants.SHARE_INSTANCE_RULES_SYNCING): + share_dict['access_rules_status'] = constants.STATUS_OUT_OF_SYNC + def _list_view(self, func, request, shares): """Provide a view for a list of shares.""" shares_list = [func(request, share)['share'] for share in shares] diff --git a/manila/common/constants.py b/manila/common/constants.py index b8c7fc8632..efe5d5fbfa 100644 --- a/manila/common/constants.py +++ b/manila/common/constants.py @@ -13,18 +13,14 @@ # License for the specific language governing permissions and limitations # under the License. -STATUS_NEW = 'new' +# SHARE AND GENERAL STATUSES STATUS_CREATING = 'creating' STATUS_DELETING = 'deleting' STATUS_DELETED = 'deleted' STATUS_ERROR = 'error' STATUS_ERROR_DELETING = 'error_deleting' STATUS_AVAILABLE = 'available' -STATUS_ACTIVE = 'active' STATUS_INACTIVE = 'inactive' -STATUS_OUT_OF_SYNC = 'out_of_sync' -STATUS_UPDATING = 'updating' -STATUS_UPDATING_MULTIPLE = 'updating_multiple' STATUS_MANAGING = 'manage_starting' STATUS_MANAGE_ERROR = 'manage_error' STATUS_UNMANAGING = 'unmanage_starting' @@ -44,6 +40,23 @@ STATUS_RESTORING = 'restoring' STATUS_REVERTING = 'reverting' STATUS_REVERTING_ERROR = 'reverting_error' +# Access rule states +ACCESS_STATE_QUEUED_TO_APPLY = 'queued_to_apply' +ACCESS_STATE_QUEUED_TO_DENY = 'queued_to_deny' +ACCESS_STATE_APPLYING = 'applying' +ACCESS_STATE_DENYING = 'denying' +ACCESS_STATE_ACTIVE = 'active' +ACCESS_STATE_ERROR = 'error' + +# Share instance "access_rules_status" field values +SHARE_INSTANCE_RULES_SYNCING = 'syncing' +SHARE_INSTANCE_RULES_ERROR = 'error' + +# States/statuses for multiple resources +STATUS_NEW = 'new' +STATUS_OUT_OF_SYNC = 'out_of_sync' +STATUS_ACTIVE = 'active' + TASK_STATE_MIGRATION_STARTING = 'migration_starting' TASK_STATE_MIGRATION_IN_PROGRESS = 'migration_in_progress' TASK_STATE_MIGRATION_COMPLETING = 'migration_completing' @@ -87,9 +100,8 @@ TRANSITIONAL_STATUSES = ( STATUS_RESTORING, STATUS_REVERTING, ) -UPDATING_RULES_STATUSES = ( - STATUS_UPDATING, - STATUS_UPDATING_MULTIPLE, +INVALID_SHARE_INSTANCE_STATUSES_FOR_ACCESS_RULE_UPDATES = ( + TRANSITIONAL_STATUSES + (STATUS_ERROR,) ) SUPPORTED_SHARE_PROTOCOLS = ( diff --git a/manila/data/helper.py b/manila/data/helper.py index d7a9a3d21b..4f40649520 100644 --- a/manila/data/helper.py +++ b/manila/data/helper.py @@ -22,6 +22,7 @@ from oslo_log import log from manila.common import constants from manila import exception from manila.i18n import _, _LW +from manila.share import access as access_manager from manila.share import rpcapi as share_rpc from manila import utils @@ -66,14 +67,13 @@ class DataServiceHelper(object): self.share = share self.context = context self.share_rpc = share_rpc.ShareAPI() + self.access_helper = access_manager.ShareInstanceAccess(self.db, None) self.wait_access_rules_timeout = ( CONF.data_access_wait_access_rules_timeout) def deny_access_to_data_service(self, access_ref_list, share_instance): - - for access_ref in access_ref_list: - self._change_data_access_to_instance( - share_instance, access_ref, allow=False) + self._change_data_access_to_instance( + share_instance, access_ref_list, deny=True) # NOTE(ganso): Cleanup methods do not throw exceptions, since the # exceptions that should be thrown are the ones that call the cleanup @@ -114,16 +114,22 @@ class DataServiceHelper(object): 'instance_id': share_instance_id, 'share_id': self.share['id']}) - def _change_data_access_to_instance( - self, instance, access_ref, allow=False): + def _change_data_access_to_instance(self, instance, accesses, deny=False): + if not isinstance(accesses, list): + accesses = [accesses] - self.db.share_instance_update_access_status( - self.context, instance['id'], constants.STATUS_OUT_OF_SYNC) + self.access_helper.get_and_update_share_instance_access_rules_status( + self.context, status=constants.SHARE_INSTANCE_RULES_SYNCING, + share_instance_id=instance['id']) - if allow: - self.share_rpc.allow_access(self.context, instance, access_ref) - else: - self.share_rpc.deny_access(self.context, instance, access_ref) + if deny: + access_filters = {'access_id': [a['id'] for a in accesses]} + updates = {'state': constants.ACCESS_STATE_QUEUED_TO_DENY} + self.access_helper.get_and_update_share_instance_access_rules( + self.context, filters=access_filters, updates=updates, + share_instance_id=instance['id']) + + self.share_rpc.update_access(self.context, instance) utils.wait_for_access_update( self.context, self.db, instance, self.wait_access_rules_timeout) @@ -166,20 +172,19 @@ class DataServiceHelper(object): self.context, self.share['id'], access['access_type'], access['access_to']) - for old_access in old_access_list: - self._change_data_access_to_instance( - share_instance, old_access, allow=False) - + # Create new access rule and deny all old ones corresponding to + # the mapping. Since this is a bulk update, all access changes + # are made in one go. access_ref = self.db.share_instance_access_create( self.context, values, share_instance['id']) self._change_data_access_to_instance( - share_instance, access_ref, allow=True) + share_instance, old_access_list, deny=True) if allow_access_to_destination_instance: access_ref = self.db.share_instance_access_create( self.context, values, dest_share_instance['id']) self._change_data_access_to_instance( - dest_share_instance, access_ref, allow=True) + dest_share_instance, access_ref) access_ref_list.append(access_ref) diff --git a/manila/db/api.py b/manila/db/api.py index 532f2bf0d1..a03a36386d 100644 --- a/manila/db/api.py +++ b/manila/db/api.py @@ -392,6 +392,31 @@ def share_access_create(context, values): return IMPL.share_access_create(context, values) +def share_access_get(context, access_id): + """Get share access rule.""" + return IMPL.share_access_get(context, access_id) + + +def share_access_get_all_for_share(context, share_id): + """Get all access rules for given share.""" + return IMPL.share_access_get_all_for_share(context, share_id) + + +def share_access_get_all_for_instance(context, instance_id, filters=None, + with_share_access_data=True): + """Get all access rules related to a certain share instance.""" + return IMPL.share_access_get_all_for_instance( + context, instance_id, filters=filters, + with_share_access_data=with_share_access_data) + + +def share_access_get_all_by_type_and_access(context, share_id, access_type, + access): + """Returns share access by given type and access.""" + return IMPL.share_access_get_all_by_type_and_access( + context, share_id, access_type, access) + + def share_instance_access_create(context, values, share_instance_id): """Allow access to share instance.""" return IMPL.share_instance_access_create( @@ -407,55 +432,24 @@ def share_instance_access_copy(context, share_id, instance_id): return IMPL.share_instance_access_copy(context, share_id, instance_id) -def share_access_get(context, access_id): - """Get share access rule.""" - return IMPL.share_access_get(context, access_id) - - -def share_instance_access_get(context, access_id, instance_id): +def share_instance_access_get(context, access_id, instance_id, + with_share_access_data=True): """Get access rule mapping for share instance.""" - return IMPL.share_instance_access_get(context, access_id, instance_id) + return IMPL.share_instance_access_get( + context, access_id, instance_id, + with_share_access_data=with_share_access_data) -def share_access_get_all_for_share(context, share_id): - """Get all access rules for given share.""" - 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( - context, instance_id, session=None) - - -def share_access_get_all_by_type_and_access(context, share_id, access_type, - access): - """Returns share access by given type and access.""" - return IMPL.share_access_get_all_by_type_and_access( - context, share_id, access_type, access) - - -def share_access_delete(context, access_id): - """Deny access to share.""" - return IMPL.share_access_delete(context, access_id) +def share_instance_access_update(context, access_id, instance_id, updates): + """Update the access mapping row for a given share instance and access.""" + return IMPL.share_instance_access_update( + context, access_id, instance_id, updates) def share_instance_access_delete(context, mapping_id): """Deny access to share instance.""" return IMPL.share_instance_access_delete(context, mapping_id) - -def share_instance_update_access_status(context, share_instance_id, status): - """Update access rules status of share instance.""" - return IMPL.share_instance_update_access_status(context, share_instance_id, - status) - - #################### diff --git a/manila/db/migrations/alembic/versions/344c1ac4747f_add_share_instance_access_rules_status.py b/manila/db/migrations/alembic/versions/344c1ac4747f_add_share_instance_access_rules_status.py index 1fd5215cf5..2f9c73fc0c 100644 --- a/manila/db/migrations/alembic/versions/344c1ac4747f_add_share_instance_access_rules_status.py +++ b/manila/db/migrations/alembic/versions/344c1ac4747f_add_share_instance_access_rules_status.py @@ -42,14 +42,6 @@ upgrade_data_mapping = { 'error': 'error', } -downgrade_data_mapping = { - 'active': 'active', - # NOTE(u_glide): We cannot determine is it applied rule or not in Manila, - # so administrator should manually handle such access rules. - 'out_of_sync': 'error', - 'error': 'error', -} - def upgrade(): """Transform individual access rules states to 'access_rules_status'. @@ -120,7 +112,12 @@ def downgrade(): for instance in connection.execute(instances_query): - state = downgrade_data_mapping[instance['access_rules_status']] + # NOTE(u_glide): We cannot determine if a rule is applied or not in + # Manila, so administrator should manually handle such access rules. + if instance['access_rules_status'] == 'active': + state = 'active' + else: + state = 'error' op.execute( instance_access_table.update().where( diff --git a/manila/db/migrations/alembic/versions/54667b9cade7_restore_share_instance_access_map_state.py b/manila/db/migrations/alembic/versions/54667b9cade7_restore_share_instance_access_map_state.py new file mode 100644 index 0000000000..bcb7d69bb7 --- /dev/null +++ b/manila/db/migrations/alembic/versions/54667b9cade7_restore_share_instance_access_map_state.py @@ -0,0 +1,99 @@ +# 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_share_instance_access_map_state + +Revision ID: 54667b9cade7 +Revises: 87ce15c59bbe +Create Date: 2016-09-02 10:18:07.290461 + +""" + +# revision identifiers, used by Alembic. +revision = '54667b9cade7' +down_revision = '87ce15c59bbe' + +from alembic import op +from sqlalchemy import Column, String + +from manila.common import constants +from manila.db.migrations import utils + + +# Mapping for new value to be assigned as ShareInstanceAccessMapping's state +access_rules_status_to_state_mapping = { + constants.STATUS_ACTIVE: constants.ACCESS_STATE_ACTIVE, + constants.STATUS_OUT_OF_SYNC: constants.ACCESS_STATE_QUEUED_TO_APPLY, + 'updating': constants.ACCESS_STATE_QUEUED_TO_APPLY, + 'updating_multiple': constants.ACCESS_STATE_QUEUED_TO_APPLY, + constants.STATUS_ERROR: constants.ACCESS_STATE_ERROR, +} + +# Mapping for changes to Share Instance's access_rules_status +access_rules_status_upgrade_mapping = { + constants.STATUS_ACTIVE: constants.STATUS_ACTIVE, + constants.STATUS_OUT_OF_SYNC: constants.SHARE_INSTANCE_RULES_SYNCING, + 'updating': constants.SHARE_INSTANCE_RULES_SYNCING, + 'updating_multiple': constants.SHARE_INSTANCE_RULES_SYNCING, + constants.STATUS_ERROR: constants.STATUS_ERROR, +} + + +def upgrade(): + op.add_column('share_instance_access_map', + Column('state', String(length=255), + default=constants.ACCESS_STATE_QUEUED_TO_APPLY)) + + connection = op.get_bind() + share_instances_table = utils.load_table('share_instances', connection) + instance_access_map_table = utils.load_table('share_instance_access_map', + connection) + + instances_query = ( + share_instances_table.select().where( + share_instances_table.c.status == + constants.STATUS_AVAILABLE).where( + share_instances_table.c.deleted == 'False') + ) + + for instance in connection.execute(instances_query): + access_rule_status = instance['access_rules_status'] + op.execute( + instance_access_map_table.update().where( + instance_access_map_table.c.share_instance_id == instance['id'] + ).values({ + 'state': access_rules_status_to_state_mapping[ + access_rule_status], + }) + ) + op.execute( + share_instances_table.update().where( + share_instances_table.c.id == instance['id'] + ).values({ + 'access_rules_status': access_rules_status_upgrade_mapping[ + access_rule_status], + }) + ) + + +def downgrade(): + op.drop_column('share_instance_access_map', 'state') + + connection = op.get_bind() + share_instances_table = utils.load_table('share_instances', connection) + + op.execute( + share_instances_table.update().where( + share_instances_table.c.access_rules_status == + constants.SHARE_INSTANCE_RULES_SYNCING).values({ + 'access_rules_status': constants.STATUS_OUT_OF_SYNC}) + ) diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index 2935cdf2ba..bada7429c0 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -38,7 +38,6 @@ from oslo_log import log from oslo_utils import timeutils from oslo_utils import uuidutils import six -from sqlalchemy import and_ from sqlalchemy import MetaData from sqlalchemy import or_ from sqlalchemy.orm import joinedload @@ -1084,25 +1083,25 @@ def reservation_expire(context): ################ -def _extract_instance_values(values_to_extract, fields): - values = copy.deepcopy(values_to_extract) - instance_values = {} +def _extract_subdict_by_fields(source_dict, fields): + dict_to_extract_from = copy.deepcopy(source_dict) + sub_dict = {} for field in fields: - field_value = values.pop(field, None) + field_value = dict_to_extract_from.pop(field, None) if field_value: - instance_values.update({field: field_value}) + sub_dict.update({field: field_value}) - return instance_values, values + return sub_dict, dict_to_extract_from def _extract_share_instance_values(values): share_instance_model_fields = [ 'status', 'host', 'scheduled_at', 'launched_at', 'terminated_at', 'share_server_id', 'share_network_id', 'availability_zone', - 'replica_state', 'share_type_id', 'share_type', + 'replica_state', 'share_type_id', 'share_type', 'access_rules_status', ] share_instance_values, share_values = ( - _extract_instance_values(values, share_instance_model_fields) + _extract_subdict_by_fields(values, share_instance_model_fields) ) return share_instance_values, share_values @@ -1110,7 +1109,7 @@ def _extract_share_instance_values(values): def _extract_snapshot_instance_values(values): fields = ['status', 'progress', 'provider_location'] snapshot_instance_values, snapshot_values = ( - _extract_instance_values(values, fields) + _extract_subdict_by_fields(values, fields) ) return snapshot_instance_values, snapshot_values @@ -1756,8 +1755,9 @@ def share_instance_access_copy(context, share_id, instance_id, session=None): """Copy access rules from share to share instance.""" session = session or get_session() - share_access_rules = share_access_get_all_for_share( - context, share_id, session=session) + share_access_rules = _share_access_get_query( + context, session, {'share_id': share_id}).all() + for access_rule in share_access_rules: values = { 'share_instance_id': instance_id, @@ -1777,9 +1777,9 @@ def _share_instance_access_create(values, session): @require_context -def share_access_get(context, access_id): +def share_access_get(context, access_id, session=None): """Get access record.""" - session = get_session() + session = session or get_session() access = _share_access_get_query( context, session, {'id': access_id}).first() @@ -1790,43 +1790,63 @@ def share_access_get(context, access_id): @require_context -def share_instance_access_get(context, access_id, instance_id): +def share_instance_access_get(context, access_id, instance_id, + with_share_access_data=True): """Get access record.""" session = get_session() access = _share_instance_access_query(context, session, access_id, instance_id).first() - if access: - return access - else: + if access is None: raise exception.NotFound() + if with_share_access_data: + access = _set_instances_share_access_data(context, access, session)[0] + + return access + @require_context def share_access_get_all_for_share(context, share_id, session=None): session = session or get_session() - return _share_access_get_query(context, session, - {'share_id': share_id}).all() + return _share_access_get_query( + context, session, {'share_id': share_id}).filter( + models.ShareAccessMapping.instance_mappings.any()).all() @require_context -def share_access_get_all_for_instance(context, instance_id, session=None): +def share_access_get_all_for_instance(context, instance_id, filters=None, + with_share_access_data=True, + session=None): """Get all access rules related to a certain share instance.""" - session = get_session() - return _share_access_get_query(context, session, {}).join( - models.ShareInstanceAccessMapping, - models.ShareInstanceAccessMapping.access_id == - models.ShareAccessMapping.id).filter(and_( - models.ShareInstanceAccessMapping.share_instance_id == - instance_id, models.ShareInstanceAccessMapping.deleted == - "False")).all() + session = session or get_session() + filters = copy.deepcopy(filters) if filters else {} + filters.update({'share_instance_id': instance_id}) + legal_filter_keys = ('id', 'share_instance_id', 'access_id', 'state') + query = _share_instance_access_query(context, session) + + query = exact_filter( + query, models.ShareInstanceAccessMapping, filters, legal_filter_keys) + + instance_accesses = query.all() + + if with_share_access_data: + instance_accesses = _set_instances_share_access_data( + context, instance_accesses, session) + + return instance_accesses -@require_context -def share_instance_access_get_all(context, access_id, session=None): - if not session: - session = get_session() - return _share_instance_access_query(context, session, access_id).all() +def _set_instances_share_access_data(context, instance_accesses, session): + if instance_accesses and not isinstance(instance_accesses, list): + instance_accesses = [instance_accesses] + + for instance_access in instance_accesses: + share_access = share_access_get( + context, instance_access['access_id'], session=session) + instance_access.set_share_access_data(share_access) + + return instance_accesses @require_context @@ -1839,22 +1859,6 @@ def share_access_get_all_by_type_and_access(context, share_id, access_type, 'access_to': access}).all() -@require_context -def share_access_delete(context, access_id): - session = get_session() - - with session.begin(): - mappings = share_instance_access_get_all(context, access_id, session) - - if len(mappings) > 0: - msg = (_("Access rule %s has mappings" - " to share instances.") % access_id) - raise exception.InvalidShareAccess(msg) - - session.query(models.ShareAccessMapping).\ - filter_by(id=access_id).soft_delete() - - @require_context def share_access_delete_all_by_share(context, share_id): session = get_session() @@ -1863,17 +1867,6 @@ 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() @@ -1885,10 +1878,11 @@ def share_instance_access_delete(context, mapping_id): if not mapping: exception.NotFound() - mapping.soft_delete(session) + mapping.soft_delete(session, update_status=True, + status_field_name='state') - other_mappings = share_instance_access_get_all( - context, mapping['access_id'], session) + other_mappings = _share_instance_access_query( + context, session, mapping['access_id']).all() # NOTE(u_glide): Remove access rule if all mappings were removed. if len(other_mappings) == 0: @@ -1901,15 +1895,27 @@ def share_instance_access_delete(context, mapping_id): @require_context @oslo_db_api.wrap_db_retry(max_retries=5, retry_on_deadlock=True) -def share_instance_update_access_status(context, share_instance_id, status): +def share_instance_access_update(context, access_id, instance_id, updates): session = get_session() - with session.begin(): - mapping = session.query(models.ShareInstance).\ - filter_by(id=share_instance_id).first() - mapping.update({'access_rules_status': status}) - mapping.save(session=session) - return mapping + share_access_fields = ('access_type', 'access_to', 'access_key', + 'access_level') + share_access_map_updates, share_instance_access_map_updates = ( + _extract_subdict_by_fields(updates, share_access_fields) + ) + + with session.begin(): + share_access = _share_access_get_query( + context, session, {'id': access_id}).first() + share_access.update(share_access_map_updates) + share_access.save(session=session) + + access = _share_instance_access_query( + context, session, access_id, instance_id).first() + access.update(share_instance_access_map_updates) + access.save(session=session) + + return access ################### diff --git a/manila/db/sqlalchemy/models.py b/manila/db/sqlalchemy/models.py index caae19335e..d560d99adb 100644 --- a/manila/db/sqlalchemy/models.py +++ b/manila/db/sqlalchemy/models.py @@ -364,17 +364,13 @@ class ShareInstance(BASE, ManilaBase): ACCESS_STATUS_PRIORITIES = { constants.STATUS_ACTIVE: 0, - constants.STATUS_OUT_OF_SYNC: 1, - constants.STATUS_UPDATING: 2, - constants.STATUS_UPDATING_MULTIPLE: 3, - constants.STATUS_ERROR: 4, + constants.SHARE_INSTANCE_RULES_SYNCING: 1, + constants.SHARE_INSTANCE_RULES_ERROR: 2, } access_rules_status = Column(Enum(constants.STATUS_ACTIVE, - constants.STATUS_OUT_OF_SYNC, - constants.STATUS_UPDATING, - constants.STATUS_UPDATING_MULTIPLE, - constants.STATUS_ERROR), + constants.SHARE_INSTANCE_RULES_SYNCING, + constants.SHARE_INSTANCE_RULES_ERROR), default=constants.STATUS_ACTIVE) scheduled_at = Column(DateTime) @@ -546,6 +542,28 @@ class ShareAccessMapping(BASE, ManilaBase): access_level = Column(Enum(*constants.ACCESS_LEVELS), default=constants.ACCESS_LEVEL_RW) + @property + def state(self): + """Get the aggregated 'state' from all the instance mapping states. + + An access rule is supposed to be truly 'active' when it has been + applied across all of the share instances of the parent share object. + """ + state = None + if len(self.instance_mappings) > 0: + order = (constants.ACCESS_STATE_ERROR, + constants.ACCESS_STATE_DENYING, + constants.ACCESS_STATE_QUEUED_TO_DENY, + constants.ACCESS_STATE_QUEUED_TO_APPLY, + constants.ACCESS_STATE_APPLYING, + constants.ACCESS_STATE_ACTIVE) + + sorted_instance_mappings = sorted( + self.instance_mappings, key=lambda x: order.index(x['state'])) + + state = sorted_instance_mappings[0].state + return state + instance_mappings = orm.relationship( "ShareInstanceAccessMapping", lazy='immediate', @@ -557,28 +575,23 @@ class ShareAccessMapping(BASE, ManilaBase): ) ) - @property - def state(self): - instances = [im.instance for im in self.instance_mappings] - access_rules_status = get_access_rules_status(instances) - - if access_rules_status in ( - constants.STATUS_OUT_OF_SYNC, - constants.STATUS_UPDATING, - constants.STATUS_UPDATING_MULTIPLE): - return constants.STATUS_NEW - else: - return access_rules_status - class ShareInstanceAccessMapping(BASE, ManilaBase): """Represents access to individual share instances.""" __tablename__ = 'share_instance_access_map' + _proxified_properties = ('share_id', 'access_type', 'access_key', + 'access_to', 'access_level') + + def set_share_access_data(self, share_access): + for share_access_attr in self._proxified_properties: + setattr(self, share_access_attr, share_access[share_access_attr]) + id = Column(String(36), primary_key=True) deleted = Column(String(36), default='False') share_instance_id = Column(String(36), ForeignKey('share_instances.id')) access_id = Column(String(36), ForeignKey('share_access_map.id')) + state = Column(String(255), default=constants.ACCESS_STATE_QUEUED_TO_APPLY) instance = orm.relationship( "ShareInstance", @@ -1019,7 +1032,7 @@ def get_access_rules_status(instances): share_access_status): share_access_status = instance_access_status - if share_access_status == constants.STATUS_ERROR: + if share_access_status == constants.SHARE_INSTANCE_RULES_ERROR: break return share_access_status diff --git a/manila/share/access.py b/manila/share/access.py index ec49f05a4b..c6949a14e8 100644 --- a/manila/share/access.py +++ b/manila/share/access.py @@ -13,218 +13,482 @@ # License for the specific language governing permissions and limitations # under the License. +import copy + from oslo_log import log -import six from manila.common import constants -from manila import exception from manila.i18n import _, _LI from manila import utils LOG = log.getLogger(__name__) -class ShareInstanceAccess(object): +def locked_access_rules_operation(operation): + """Lock decorator for access rules operations. + + Takes a named lock prior to executing the operation. The lock is + named with the ID of the share instance to which the access rule belongs. + + Intended use: + If an database operation to retrieve or update access rules uses this + decorator, it will block actions on all access rules of the share + instance until the named lock is free. This is used to avoid race + conditions while performing access rules updates on a given share instance. + """ + + def wrapped(*args, **kwargs): + instance_id = kwargs.get('share_instance_id') + + @utils.synchronized( + "locked_access_rules_operation_by_share_instance_%s" % instance_id, + external=True) + def locked_operation(*_args, **_kwargs): + return operation(*_args, **_kwargs) + + return locked_operation(*args, **kwargs) + + return wrapped + + +class ShareInstanceAccessDatabaseMixin(object): + + @locked_access_rules_operation + def get_and_update_share_instance_access_rules_status( + self, context, status=None, conditionally_change={}, + share_instance_id=None): + """Get and update the access_rules_status of a share instance. + + :param status: Set this parameter only if you want to + omit the conditionally_change parameter; i.e, if you want to + force a state change on the share instance regardless of the prior + state. + :param conditionally_change: Set this parameter to a dictionary of rule + state transitions to be made. The key is the expected + access_rules_status and the value is the state to transition the + access_rules_status to. If the state is not as expected, + no transition is performed. Default is {}, which means no state + transitions will be made. + :returns share_instance: if an update was made. + """ + if status is not None: + updates = {'access_rules_status': status} + else: + share_instance = self.db.share_instance_get( + context, share_instance_id) + access_rules_status = share_instance['access_rules_status'] + try: + updates = { + 'access_rules_status': + conditionally_change[access_rules_status], + } + except KeyError: + updates = {} + if updates: + share_instance = self.db.share_instance_update( + context, share_instance_id, updates, with_share_data=True) + return share_instance + + @locked_access_rules_operation + def get_and_update_share_instance_access_rules(self, context, + filters={}, updates={}, + conditionally_change={}, + share_instance_id=None): + """Get and conditionally update all access rules of a share instance. + + :param updates: Set this parameter to a dictionary of key:value + pairs corresponding to the keys in the ShareInstanceAccessMapping + model. Include 'state' in this dictionary only if you want to + omit the conditionally_change parameter; i.e, if you want to + force a state change on all filtered rules regardless of the prior + state. This parameter is always honored, regardless of whether + conditionally_change allows for a state transition as desired. + + Example:: + + { + 'access_key': 'bob007680048318f4239dfc1c192d5', + 'access_level': 'ro', + } + + :param conditionally_change: Set this parameter to a dictionary of rule + state transitions to be made. The key is the expected state of + the access rule the value is the state to transition the + access rule to. If the state is not as expected, no transition is + performed. Default is {}, which means no state transitions + will be made. + + Example:: + + { + 'queued_to_apply': 'applying', + 'queued_to_deny': 'denying', + } + + """ + instance_rules = self.db.share_access_get_all_for_instance( + context, share_instance_id, filters=filters) + + if instance_rules and (updates or conditionally_change): + for rule in instance_rules: + mapping_state = rule['state'] + rule_updates = copy.deepcopy(updates) + try: + rule_updates['state'] = conditionally_change[mapping_state] + except KeyError: + pass + if rule_updates: + self.db.share_instance_access_update( + context, rule['access_id'], share_instance_id, + rule_updates) + + # Refresh the rules after the updates + rules_to_get = { + 'access_id': tuple([i['access_id'] for i in instance_rules]), + } + instance_rules = self.db.share_access_get_all_for_instance( + context, share_instance_id, filters=rules_to_get) + + return instance_rules + + @locked_access_rules_operation + def get_and_update_share_instance_access_rule(self, context, rule_id, + updates={}, + share_instance_id=None, + conditionally_change={}): + """Get and conditionally update a given share instance access rule. + + :param updates: Set this parameter to a dictionary of key:value + pairs corresponding to the keys in the ShareInstanceAccessMapping + model. Include 'state' in this dictionary only if you want to + omit the conditionally_change parameter; i.e, if you want to + force a state change regardless of the prior state. + :param conditionally_change: Set this parameter to a dictionary of rule + state transitions to be made. The key is the expected state of + the access rule the value is the state to transition the + access rule to. If the state is not as expected, no transition is + performed. Default is {}, which means no state transitions + will be made. + + Example:: + + { + 'queued_to_apply': 'applying', + 'queued_to_deny': 'denying', + } + """ + instance_rule_mapping = self.db.share_instance_access_get( + context, rule_id, share_instance_id) + + if conditionally_change: + mapping_state = instance_rule_mapping['state'] + try: + updated_state = conditionally_change[mapping_state] + updates.update({'state': updated_state}) + except KeyError: + msg = ("The state of the access rule %(rule_id)s (allowing " + "access to share instance %(si)s) was not updated " + "because its state was modified by another operation.") + msg_payload = { + 'si': share_instance_id, + 'rule_id': rule_id, + } + LOG.debug(msg, msg_payload) + if updates: + self.db.share_instance_access_update( + context, rule_id, share_instance_id, updates) + + # Refresh the rule after update + instance_rule_mapping = self.db.share_instance_access_get( + context, rule_id, share_instance_id) + + return instance_rule_mapping + + @locked_access_rules_operation + def delete_share_instance_access_rules(self, context, access_rules, + share_instance_id=None): + for rule in access_rules: + self.db.share_instance_access_delete(context, rule['id']) + + +class ShareInstanceAccess(ShareInstanceAccessDatabaseMixin): def __init__(self, db, driver): self.db = db self.driver = driver - def update_access_rules(self, context, share_instance_id, add_rules=None, - delete_rules=None, share_server=None): - """Update access rules in driver and database for given share instance. + def update_access_rules(self, context, share_instance_id, + delete_all_rules=False, share_server=None): + """Update access rules for a given share instance. - :param context: current context - :param share_instance_id: Id of the share instance model - :param add_rules: list with ShareAccessMapping models or None - rules - which should be added - :param delete_rules: list with ShareAccessMapping models, "all", None - - rules which should be deleted. If "all" is provided - all rules will - be deleted. + :param context: request context + :param share_instance_id: ID of the share instance + :param delete_all_rules: set this parameter to True if all + existing access rules must be denied for a given share instance :param share_server: Share server model or None """ share_instance = self.db.share_instance_get( context, share_instance_id, with_share_data=True) - share_id = share_instance["share_id"] + msg_payload = { + 'si': share_instance_id, + 'shr': share_instance['share_id'], + } - @utils.synchronized( - "update_access_rules_for_share_%s" % share_id, external=True) - def _update_access_rules_locked(*args, **kwargs): - return self._update_access_rules(*args, **kwargs) + if delete_all_rules: + updates = { + 'state': constants.ACCESS_STATE_QUEUED_TO_DENY, + } + self.get_and_update_share_instance_access_rules( + context, updates=updates, share_instance_id=share_instance_id) - _update_access_rules_locked( - context=context, - share_instance_id=share_instance_id, - add_rules=add_rules, - delete_rules=delete_rules, - share_server=share_server, - ) + # Is there a sync in progress? If yes, ignore the incoming request. + rule_filter = { + 'state': (constants.ACCESS_STATE_APPLYING, + constants.ACCESS_STATE_DENYING), + } + syncing_rules = self.get_and_update_share_instance_access_rules( + context, filters=rule_filter, share_instance_id=share_instance_id) - def _update_access_rules(self, context, share_instance_id, add_rules=None, - delete_rules=None, share_server=None): - # Reget share instance + if syncing_rules: + msg = ("Access rules are being synced for share instance " + "%(si)s belonging to share %(shr)s, any rule changes will " + "be applied shortly.") + LOG.debug(msg, msg_payload) + else: + rules_to_apply_or_deny = ( + self._update_and_get_unsynced_access_rules_from_db( + context, share_instance_id) + ) + if rules_to_apply_or_deny: + msg = ("Updating access rules for share instance %(si)s " + "belonging to share %(shr)s.") + LOG.debug(msg, msg_payload) + self._update_access_rules(context, share_instance_id, + share_server=share_server) + else: + msg = ("All access rules have been synced for share instance " + "%(si)s belonging to share %(shr)s.") + LOG.debug(msg, msg_payload) + + def _update_access_rules(self, context, share_instance_id, + share_server=None): + # Refresh the share instance model share_instance = self.db.share_instance_get( context, share_instance_id, with_share_data=True) - # NOTE (rraja): preserve error state to trigger maintenance mode - if share_instance['access_rules_status'] != constants.STATUS_ERROR: - self.db.share_instance_update_access_status( - context, - share_instance_id, - constants.STATUS_UPDATING) + conditionally_change = { + constants.STATUS_ACTIVE: constants.SHARE_INSTANCE_RULES_SYNCING, + } + share_instance = ( + self.get_and_update_share_instance_access_rules_status( + context, conditionally_change=conditionally_change, + share_instance_id=share_instance_id) or share_instance + ) - add_rules = add_rules or [] - delete_rules = delete_rules or [] - remove_rules = None - - if six.text_type(delete_rules).lower() == "all": - # NOTE(ganso): if we are deleting an instance or clearing all - # the rules, we want to remove only the ones related - # to this instance. - delete_rules = self.db.share_access_get_all_for_instance( - context, share_instance['id']) - rules = [] - else: - _rules = self.db.share_access_get_all_for_instance( - context, share_instance['id']) - rules = _rules - if delete_rules: - delete_ids = [rule['id'] for rule in delete_rules] - rules = list(filter(lambda r: r['id'] not in delete_ids, - rules)) - # NOTE(ganso): trigger maintenance mode - if share_instance['access_rules_status'] == ( - constants.STATUS_ERROR): - remove_rules = [ - rule for rule in _rules - if rule["id"] in delete_ids] - delete_rules = [] - - # NOTE(ganso): up to here we are certain of the rules that we are - # supposed to pass to drivers. 'rules' variable is used for validating - # the refresh mechanism later, according to the 'supposed' rules. - driver_rules = rules + rules_to_be_removed_from_db = [] + # Populate rules to send to the driver + (access_rules_to_be_on_share, add_rules, delete_rules) = ( + self._get_rules_to_send_to_driver(context, share_instance) + ) if share_instance['status'] == constants.STATUS_MIGRATING: - # NOTE(ganso): If the share instance is the source in a migration, - # it should have all its rules cast to read-only. - - readonly_support = self.driver.configuration.safe_get( - 'migration_readonly_rules_support') - - # NOTE(ganso): If the backend supports read-only rules, then all - # rules are cast to read-only here and passed to drivers. - if readonly_support: - for rule in driver_rules + add_rules: - rule['access_level'] = constants.ACCESS_LEVEL_RO - LOG.debug("All access rules of share instance %s are being " - "cast to read-only for migration.", - share_instance['id']) - # NOTE(ganso): If the backend does not support read-only rules, we - # will remove all access to the share and have only the access - # requested by the Data Service for migration as RW. - else: - LOG.debug("All previously existing access rules of share " - "instance %s are being removed for migration, as " - "driver does not support read-only mode rules.", - share_instance['id']) - driver_rules = add_rules + # Ensure read/only semantics for a migrating instances + access_rules_to_be_on_share = self._set_rules_to_readonly( + add_rules, access_rules_to_be_on_share, share_instance) + add_rules = [] + rules_to_be_removed_from_db = delete_rules + delete_rules = [] try: - access_keys = None - try: - access_keys = self.driver.update_access( - context, - share_instance, - driver_rules, - add_rules=add_rules, - delete_rules=delete_rules, - share_server=share_server - ) - except NotImplementedError: - # NOTE(u_glide): Fallback to legacy allow_access/deny_access - # for drivers without update_access() method support + driver_rule_updates = self._update_rules_through_share_driver( + context, share_instance, access_rules_to_be_on_share, + add_rules, delete_rules, rules_to_be_removed_from_db, + share_server) - self._update_access_fallback(add_rules, context, delete_rules, - remove_rules, share_instance, - share_server) + self._process_driver_rule_updates( + context, driver_rule_updates, share_instance_id) - 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) + # Update access rules that are still in 'applying' state + conditionally_change = { + constants.ACCESS_STATE_APPLYING: constants.ACCESS_STATE_ACTIVE, + } + self.get_and_update_share_instance_access_rules( + context, share_instance_id=share_instance_id, + conditionally_change=conditionally_change) except Exception: - self.db.share_instance_update_access_status( - context, - share_instance['id'], - constants.STATUS_ERROR) + conditionally_change_rule_state = { + constants.ACCESS_STATE_APPLYING: constants.ACCESS_STATE_ERROR, + constants.ACCESS_STATE_DENYING: constants.ACCESS_STATE_ERROR, + } + self.get_and_update_share_instance_access_rules( + context, share_instance_id=share_instance_id, + conditionally_change=conditionally_change_rule_state) + + conditionally_change_access_rules_status = { + constants.ACCESS_STATE_ACTIVE: constants.STATUS_ERROR, + constants.SHARE_INSTANCE_RULES_SYNCING: constants.STATUS_ERROR, + } + self.get_and_update_share_instance_access_rules_status( + context, share_instance_id=share_instance_id, + conditionally_change=conditionally_change_access_rules_status) raise - # NOTE(ganso): remove rules after maintenance is complete - if remove_rules: - delete_rules = remove_rules + if rules_to_be_removed_from_db: + delete_rules = rules_to_be_removed_from_db - self._remove_access_rules(context, delete_rules, share_instance['id']) + self.delete_share_instance_access_rules( + context, delete_rules, share_instance_id=share_instance['id']) - share_instance = self.db.share_instance_get(context, share_instance_id, - with_share_data=True) + self._loop_for_refresh_else_update_access_rules_status( + context, share_instance_id, share_server) - if self._check_needs_refresh(context, rules, share_instance): + msg = _("Access rules were successfully modified for share instance " + "%(si)s belonging to share %(shr)s.") + msg_payload = { + 'si': share_instance['id'], + 'shr': share_instance['share_id'], + } + LOG.info(msg, msg_payload) + + def _update_rules_through_share_driver(self, context, share_instance, + access_rules_to_be_on_share, + add_rules, delete_rules, + rules_to_be_removed_from_db, + share_server): + driver_rule_updates = {} + try: + driver_rule_updates = self.driver.update_access( + context, + share_instance, + access_rules_to_be_on_share, + add_rules=add_rules, + delete_rules=delete_rules, + share_server=share_server + ) or {} + except NotImplementedError: + # NOTE(u_glide): Fallback to legacy allow_access/deny_access + # for drivers without update_access() method support + self._update_access_fallback(context, add_rules, delete_rules, + rules_to_be_removed_from_db, + share_instance, + share_server) + return driver_rule_updates + + def _loop_for_refresh_else_update_access_rules_status(self, context, + share_instance_id, + share_server): + # Do we need to re-sync or apply any new changes? + if self._check_needs_refresh(context, share_instance_id): self._update_access_rules(context, share_instance_id, share_server=share_server) else: - self.db.share_instance_update_access_status( - context, - share_instance['id'], - constants.STATUS_ACTIVE + # Switch the share instance's access_rules_status to 'active' + # if there are no more rules in 'error' state, else, ensure + # 'error' state. + rule_filter = {'state': constants.STATUS_ERROR} + rules_in_error_state = ( + self.get_and_update_share_instance_access_rules( + context, filters=rule_filter, + share_instance_id=share_instance_id) ) + if not rules_in_error_state: + conditionally_change = { + constants.SHARE_INSTANCE_RULES_SYNCING: + constants.STATUS_ACTIVE, + constants.SHARE_INSTANCE_RULES_ERROR: + constants.STATUS_ACTIVE, + } + self.get_and_update_share_instance_access_rules_status( + context, conditionally_change=conditionally_change, + share_instance_id=share_instance_id) + else: + conditionally_change = { + constants.SHARE_INSTANCE_RULES_SYNCING: + constants.SHARE_INSTANCE_RULES_ERROR, + } + self.get_and_update_share_instance_access_rules_status( + context, conditionally_change=conditionally_change, + share_instance_id=share_instance_id) - LOG.info(_LI("Access rules were successfully applied for " - "share instance: %s"), - share_instance['id']) + def _process_driver_rule_updates(self, context, driver_rule_updates, + share_instance_id): + for rule_id, rule_updates in driver_rule_updates.items(): + if 'state' in rule_updates: + # We allow updates *only* if the state is unchanged from + # the time this update was initiated. It is possible + # that the access rule was denied at the API prior to + # the driver reporting that the access rule was added + # successfully. + state = rule_updates.pop('state') + conditional_state_updates = { + constants.ACCESS_STATE_APPLYING: state, + constants.ACCESS_STATE_DENYING: state, + constants.ACCESS_STATE_ACTIVE: state, + } + else: + conditional_state_updates = {} + self.get_and_update_share_instance_access_rule( + context, rule_id, updates=rule_updates, + share_instance_id=share_instance_id, + conditionally_change=conditional_state_updates) - @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] + def _set_rules_to_readonly(self, add_rules, access_rules_to_be_on_share, + share_instance): + # NOTE(ganso): If the share instance is the source in a migration, + # it should have all its rules cast to read-only. + readonly_support = self.driver.configuration.safe_get( + 'migration_readonly_rules_support') + # NOTE(ganso): If the backend supports read-only rules, then all + # rules are cast to read-only here and passed to drivers. + if readonly_support: + for rule in access_rules_to_be_on_share: + rule['access_level'] = constants.ACCESS_LEVEL_RO + LOG.debug("All access rules of share instance %s are being " + "cast to read-only for migration.", + share_instance['id']) + # NOTE(ganso): If the backend does not support read-only rules, we + # will remove all access to the share and have only the access + # requested by the Data Service for migration as RW. 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) + LOG.debug("All previously existing access rules of share " + "instance %s are being removed for migration, as " + "driver does not support read-only mode rules.", + share_instance['id']) + access_rules_to_be_on_share = add_rules + return access_rules_to_be_on_share - 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 _get_rules_to_send_to_driver(self, context, share_instance): + add_rules = [] + delete_rules = [] + access_filters = { + 'state': (constants.ACCESS_STATE_APPLYING, + constants.ACCESS_STATE_ACTIVE, + constants.ACCESS_STATE_DENYING), + } + existing_rules_in_db = self.get_and_update_share_instance_access_rules( + context, filters=access_filters, + share_instance_id=share_instance['id']) + # Update queued rules to transitional states + for rule in existing_rules_in_db: + if rule['state'] == constants.ACCESS_STATE_APPLYING: + add_rules.append(rule) + elif rule['state'] == constants.ACCESS_STATE_DENYING: + delete_rules.append(rule) + delete_rule_ids = [r['id'] for r in delete_rules] + access_rules_to_be_on_share = [ + r for r in existing_rules_in_db if r['id'] not in delete_rule_ids + ] + return access_rules_to_be_on_share, add_rules, delete_rules - 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( - context, share_instance['id']) - queried_ids = set([rule['id'] for rule in queried_rules]) + def _check_needs_refresh(self, context, share_instance_id): + rules_to_apply_or_deny = ( + self._update_and_get_unsynced_access_rules_from_db( + context, share_instance_id) + ) + return any(rules_to_apply_or_deny) - access_rules_status = share_instance['access_rules_status'] - - return (access_rules_status == constants.STATUS_UPDATING_MULTIPLE or - rule_ids != queried_ids) - - def _update_access_fallback(self, add_rules, context, delete_rules, + def _update_access_fallback(self, context, add_rules, delete_rules, remove_rules, share_instance, share_server): for rule in add_rules: LOG.info( @@ -242,7 +506,8 @@ class ShareInstanceAccess(object): # NOTE(ganso): Fallback mode temporary compatibility workaround if remove_rules: - delete_rules = remove_rules + delete_rules.extend(remove_rules) + for rule in delete_rules: LOG.info( _LI("Denying access rule '%(rule)s' from share " @@ -257,12 +522,33 @@ class ShareInstanceAccess(object): share_server=share_server ) - def _remove_access_rules(self, context, access_rules, share_instance_id): - if not access_rules: - return + def _update_and_get_unsynced_access_rules_from_db(self, context, + share_instance_id): + rule_filter = { + 'state': (constants.ACCESS_STATE_QUEUED_TO_APPLY, + constants.ACCESS_STATE_QUEUED_TO_DENY), + } + conditionally_change = { + constants.ACCESS_STATE_QUEUED_TO_APPLY: + constants.ACCESS_STATE_APPLYING, + constants.ACCESS_STATE_QUEUED_TO_DENY: + constants.ACCESS_STATE_DENYING, + } + rules_to_apply_or_deny = ( + self.get_and_update_share_instance_access_rules( + context, filters=rule_filter, + share_instance_id=share_instance_id, + conditionally_change=conditionally_change) + ) + return rules_to_apply_or_deny - for rule in access_rules: - access_mapping = self.db.share_instance_access_get( - context, rule['id'], share_instance_id) - - self.db.share_instance_access_delete(context, access_mapping['id']) + def reset_applying_rules(self, context, share_instance_id): + conditional_updates = { + constants.ACCESS_STATE_APPLYING: + constants.ACCESS_STATE_QUEUED_TO_APPLY, + constants.ACCESS_STATE_DENYING: + constants.ACCESS_STATE_QUEUED_TO_DENY, + } + self.get_and_update_share_instance_access_rules( + context, share_instance_id=share_instance_id, + conditionally_change=conditional_updates) diff --git a/manila/share/api.py b/manila/share/api.py index 04527cc4c8..a797c3dc43 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -36,6 +36,7 @@ from manila.i18n import _, _LE, _LI, _LW from manila import policy from manila import quota from manila.scheduler import rpcapi as scheduler_rpcapi +from manila.share import access from manila.share import rpcapi as share_rpcapi from manila.share import share_types from manila.share import utils as share_utils @@ -61,9 +62,10 @@ class API(base.Base): """API for interacting with the share manager.""" def __init__(self, db_driver=None): + super(API, self).__init__(db_driver) self.scheduler_rpcapi = scheduler_rpcapi.SchedulerAPI() self.share_rpcapi = share_rpcapi.ShareAPI() - super(API, self).__init__(db_driver) + self.access_helper = access.ShareInstanceAccess(self.db, None) def create(self, context, share_proto, size, name, description, snapshot_id=None, availability_zone=None, metadata=None, @@ -1513,14 +1515,35 @@ class API(base.Base): """Get the newest snapshot of a share.""" return self.db.share_snapshot_get_latest_for_share(context, share_id) + @staticmethod + def _is_invalid_share_instance(instance): + return (instance['host'] is None + or instance['status'] in constants. + INVALID_SHARE_INSTANCE_STATUSES_FOR_ACCESS_RULE_UPDATES) + def allow_access(self, ctx, share, access_type, access_to, access_level=None): """Allow access to share.""" - policy.check_policy(ctx, 'share', 'allow_access') - share = self.db.share_get(ctx, share['id']) - if share['status'] != constants.STATUS_AVAILABLE: - msg = _("Share status must be %s") % constants.STATUS_AVAILABLE - raise exception.InvalidShare(reason=msg) + + # Access rule validation: + if access_level not in constants.ACCESS_LEVELS + (None, ): + msg = _("Invalid share access level: %s.") % access_level + raise exception.InvalidShareAccess(reason=msg) + share_access_list = self.db.share_access_get_all_by_type_and_access( + ctx, share['id'], access_type, access_to) + + if len(share_access_list) > 0: + raise exception.ShareAccessExists(access_type=access_type, + access=access_to) + + # Share instance validation + if any(instance for instance in share.instances + if self._is_invalid_share_instance(instance)): + msg = _("New access rules cannot be applied while the share or " + "any of its replicas or migration copies lacks a valid " + "host or is in an invalid state.") + raise exception.InvalidShare(message=msg) + values = { 'share_id': share['id'], 'access_type': access_type, @@ -1528,101 +1551,49 @@ class API(base.Base): 'access_level': access_level, } - share_access_list = self.db.share_access_get_all_by_type_and_access( - ctx, share['id'], access_type, access_to) - - if len(share_access_list) > 0: - raise exception.ShareAccessExists(access_type=access_type, - access=access_to) - - if access_level not in constants.ACCESS_LEVELS + (None, ): - msg = _("Invalid share access level: %s.") % access_level - raise exception.InvalidShareAccess(reason=msg) access = self.db.share_access_create(ctx, values) for share_instance in share.instances: - self.allow_access_to_instance(ctx, share_instance, access) - - # NOTE(tpsilva): refreshing share_access model - access = self.db.share_access_get(ctx, access['id']) + self.allow_access_to_instance(ctx, share_instance) return access - def allow_access_to_instance(self, context, share_instance, access): - policy.check_policy(context, 'share', 'allow_access') + def allow_access_to_instance(self, context, share_instance): + self._conditionally_transition_share_instance_access_rules_status( + context, share_instance) + self.share_rpcapi.update_access(context, share_instance) - if not share_instance['host']: - msg = _("Invalid share instance host: %s") % share_instance['host'] - raise exception.InvalidShareInstance(reason=msg) - - status = share_instance['access_rules_status'] - - if status == constants.STATUS_ERROR: - values = { - 'instance_id': share_instance['id'], - 'status': status, - 'valid_status': constants.STATUS_ACTIVE - } - msg = _("Share instance %(instance_id)s access rules status is: " - "%(status)s. Please remove any incorrect rules to get it " - "back to %(valid_status)s.") % values - - raise exception.InvalidShareInstance(reason=msg) - else: - if status == constants.STATUS_ACTIVE: - self.db.share_instance_update_access_status( - context, share_instance['id'], - constants.STATUS_OUT_OF_SYNC - ) - elif status == constants.STATUS_UPDATING: - self.db.share_instance_update_access_status( - context, share_instance['id'], - constants.STATUS_UPDATING_MULTIPLE - ) - - self.share_rpcapi.allow_access(context, share_instance, access) + def _conditionally_transition_share_instance_access_rules_status( + self, context, share_instance): + conditionally_change = { + constants.STATUS_ACTIVE: constants.SHARE_INSTANCE_RULES_SYNCING, + } + self.access_helper.get_and_update_share_instance_access_rules_status( + context, conditionally_change=conditionally_change, + share_instance_id=share_instance['id']) def deny_access(self, ctx, share, access): """Deny access to share.""" - policy.check_policy(ctx, 'share', 'deny_access') - # First check state of the target share - share = self.db.share_get(ctx, share['id']) - if not (share.instances and share.instance['host']): - msg = _("Share doesn't have any instances") - raise exception.InvalidShare(reason=msg) - if share['status'] != constants.STATUS_AVAILABLE: - msg = _("Share status must be %s") % constants.STATUS_AVAILABLE - raise exception.InvalidShare(reason=msg) + + if any(instance for instance in share.instances if + self._is_invalid_share_instance(instance)): + msg = _("Access rules cannot be denied while the share, " + "any of its replicas or migration copies lacks a valid " + "host or is in an invalid state.") + raise exception.InvalidShare(message=msg) for share_instance in share.instances: - try: self.deny_access_to_instance(ctx, share_instance, access) - except exception.NotFound: - LOG.warning(_LW("Access rule %(access_id)s not found " - "for instance %(instance_id)s.") % { - 'access_id': access['id'], - 'instance_id': share_instance['id']}) def deny_access_to_instance(self, context, share_instance, access): - policy.check_policy(context, 'share', 'deny_access') + self._conditionally_transition_share_instance_access_rules_status( + context, share_instance) + updates = {'state': constants.ACCESS_STATE_QUEUED_TO_DENY} + self.access_helper.get_and_update_share_instance_access_rule( + context, access['id'], updates=updates, + share_instance_id=share_instance['id']) - if not share_instance['host']: - msg = _("Invalid share instance host: %s") % share_instance['host'] - raise exception.InvalidShareInstance(reason=msg) - - status = share_instance['access_rules_status'] - - if status != constants.STATUS_ERROR: - new_status = constants.STATUS_OUT_OF_SYNC - - if status in constants.UPDATING_RULES_STATUSES: - new_status = constants.STATUS_UPDATING_MULTIPLE - - self.db.share_instance_update_access_status( - context, share_instance['id'], - new_status) - - self.share_rpcapi.deny_access(context, share_instance, access) + self.share_rpcapi.update_access(context, share_instance) def access_get_all(self, context, share): """Returns all access rules for share.""" diff --git a/manila/share/driver.py b/manila/share/driver.py index 4d1d4ff971..268b64e474 100644 --- a/manila/share/driver.py +++ b/manila/share/driver.py @@ -607,16 +607,22 @@ class ShareDriver(object): delete_rules, share_server=None): """Update access rules for given share. - Drivers should support 2 different cases in this method: - 1. Recovery after error - 'access_rules' contains all access_rules, - 'add_rules' and 'delete_rules' shall be empty. Driver should clear any - existent access rules and apply all access rules for given share. - This recovery is made at driver start up. + ``access_rules`` contains all access_rules that need to be on the + share. If the driver can make bulk access rule updates, it can + safely ignore the ``add_rules`` and ``delete_rules`` parameters. - 2. Adding/Deleting of several access rules - 'access_rules' contains - all access_rules, 'add_rules' and 'delete_rules' contain rules which - should be added/deleted. Driver can ignore rules in 'access_rules' and - apply only rules from 'add_rules' and 'delete_rules'. + If the driver cannot make bulk access rule changes, it can rely on + new rules to be present in ``add_rules`` and rules that need to be + removed to be present in ``delete_rules``. + + When a rule in ``delete_rules`` was never applied, drivers must not + raise an exception, or attempt to set the rule to ``error`` state. + + ``add_rules`` and ``delete_rules`` can be empty lists, in this + situation, drivers should ensure that the rules present in + ``access_rules`` are the same as those on the back end. One scenario + where this situation is forced is when the access_level is changed for + all existing rules (share migration and for readable replicas). Drivers must be mindful of this call for share replicas. When 'update_access' is called on one of the replicas, the call is likely @@ -634,20 +640,51 @@ class ShareDriver(object): :param context: Current context :param share: Share model with share data. - :param access_rules: All access rules for given share + :param access_rules: A list of access rules for given share :param add_rules: Empty List or List of access rules which should be added. access_rules already contains these rules. :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. + :returns: None, or a dictionary of updates in the format: + + { + '09960614-8574-4e03-89cf-7cf267b0bd08': { + 'access_key': 'alice31493e5441b8171d2310d80e37e', + 'state': 'error', + }, + '28f6eabb-4342-486a-a7f4-45688f0c0295': { + 'access_key': 'bob0078aa042d5a7325480fd13228b', + 'state': 'active', + }, + } + + The top level keys are 'access_id' fields of the access rules that + need to be updated. ``access_key``s are credentials (str) of the + entities granted access. Any rule in the ``access_rules`` parameter + can be updated. + + .. important:: + + Raising an exception in this method will force *all* rules in + 'applying' and 'denying' states to 'error'. + + An access rule can be set to 'error' state, either explicitly + via this return parameter or because of an exception raised in + this method. Such an access rule will no longer be sent to the + driver on subsequent access rule updates. When users deny that + rule however, the driver will be asked to deny access to the + client/s represented by the rule. We expect that a + rule that was error-ed at the driver should never exist on the + back end. So, do not fail the deletion request. + + Also, it is possible that the driver may receive a request to + add a rule that is already present on the back end. + This can happen if the share manager service goes down + while the driver is committing access rule changes. Since we + cannot determine if the rule was applied successfully by the driver + before the disruption, we will treat all 'applying' transitional + rules as new rules and repeat the request. """ raise NotImplementedError() diff --git a/manila/share/drivers/cephfs/cephfs_native.py b/manila/share/drivers/cephfs/cephfs_native.py index ddce19f465..b6e8b59afa 100644 --- a/manila/share/drivers/cephfs/cephfs_native.py +++ b/manila/share/drivers/cephfs/cephfs_native.py @@ -289,7 +289,7 @@ class CephFSNativeDriver(driver.ShareDriver,): # backend are in sync. for rule in add_rules: access_key = self._allow_access(context, share, rule) - access_keys.update({rule['id']: access_key}) + access_keys.update({rule['access_id']: {'access_key': access_key}}) for rule in delete_rules: self._deny_access(context, share, rule) diff --git a/manila/share/drivers/huawei/v3/connection.py b/manila/share/drivers/huawei/v3/connection.py index b9a4fe6bad..3bcd7fa2f5 100644 --- a/manila/share/drivers/huawei/v3/connection.py +++ b/manila/share/drivers/huawei/v3/connection.py @@ -1810,8 +1810,9 @@ class V3StorageConnection(driver.HuaweiBase): 'replica_state': common_constants.REPLICA_STATE_ACTIVE, } new_active_update['access_rules_status'] = ( - common_constants.STATUS_ACTIVE if updated_new_active_access - else common_constants.STATUS_OUT_OF_SYNC) + common_constants.STATUS_ACTIVE + if updated_new_active_access + else common_constants.SHARE_INSTANCE_RULES_SYNCING) # get replica state for new secondary after switch over replica_state = self.replica_mgr.get_replica_state(replica_pair_id) @@ -1821,7 +1822,8 @@ class V3StorageConnection(driver.HuaweiBase): 'replica_state': replica_state, } old_active_update['access_rules_status'] = ( - common_constants.STATUS_OUT_OF_SYNC if cleared_old_active_access + common_constants.SHARE_INSTANCE_RULES_SYNCING + if cleared_old_active_access else common_constants.STATUS_ACTIVE) return [new_active_update, old_active_update] diff --git a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py index 85fbb48f80..d0de1e51de 100644 --- a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py +++ b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py @@ -1469,7 +1469,7 @@ class NetAppCmodeFileStorageLibrary(object): helper.update_access(replica, share_name, access_rules) except Exception: new_active_replica['access_rules_status'] = ( - constants.STATUS_OUT_OF_SYNC) + constants.SHARE_INSTANCE_RULES_SYNCING) else: new_active_replica['access_rules_status'] = constants.STATUS_ACTIVE diff --git a/manila/share/drivers/zfsonlinux/driver.py b/manila/share/drivers/zfsonlinux/driver.py index 71a325e8c9..3bb50aa1ee 100644 --- a/manila/share/drivers/zfsonlinux/driver.py +++ b/manila/share/drivers/zfsonlinux/driver.py @@ -1086,7 +1086,7 @@ class ZFSonLinuxShareDriver(zfs_utils.ExecuteMixin, driver.ShareDriver): 'id': r['id'], # NOTE(vponomaryov): access rules will be updated in next # 'sync' operation. - 'access_rules_status': constants.STATUS_OUT_OF_SYNC, + 'access_rules_status': constants.SHARE_INSTANCE_RULES_SYNCING, } for r in replica_list } diff --git a/manila/share/manager.py b/manila/share/manager.py index 84f6fa503f..c82ae4ded1 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -188,7 +188,7 @@ def add_hooks(f): class ShareManager(manager.SchedulerDependentManager): """Manages NAS storages.""" - RPC_API_VERSION = '1.13' + RPC_API_VERSION = '1.14' def __init__(self, share_driver=None, service_name=None, *args, **kwargs): """Load the driver from args, or from flags.""" @@ -218,6 +218,8 @@ class ShareManager(manager.SchedulerDependentManager): ) self.access_helper = access.ShareInstanceAccess(self.db, self.driver) + self.migration_wait_access_rules_timeout = ( + CONF.migration_wait_access_rules_timeout) self.hooks = [] self._init_hook_drivers() @@ -330,10 +332,12 @@ class ShareManager(manager.SchedulerDependentManager): self.db.share_export_locations_update( ctxt, share_instance['id'], export_locations) - if share_instance['access_rules_status'] == ( - constants.STATUS_OUT_OF_SYNC): - + if share_instance['access_rules_status'] != ( + constants.STATUS_ACTIVE): try: + # Cast any existing 'applying' rules to 'new' + self.access_helper.reset_applying_rules( + ctxt, share_instance['id']) self.access_helper.update_access_rules( ctxt, share_instance['id'], share_server=share_server) except Exception: @@ -688,7 +692,8 @@ class ShareManager(manager.SchedulerDependentManager): dest_share_instance = self.db.share_instance_get( context, dest_share_instance['id'], with_share_data=True) - helper = migration.ShareMigrationHelper(context, self.db, share_ref) + helper = migration.ShareMigrationHelper( + context, self.db, share_ref, self.access_helper) try: if dest_share_instance['share_network_id']: @@ -739,14 +744,8 @@ class ShareManager(manager.SchedulerDependentManager): raise exception.ShareMigrationFailed(reason=msg) if not compatibility.get('writable'): - - self.db.share_instance_update_access_status( - context, src_share_instance['id'], - constants.STATUS_OUT_OF_SYNC) - - self.access_helper.update_access_rules( - context, src_share_instance['id'], - share_server=share_server) + self._cast_access_rules_to_readonly( + context, src_share_instance, share_server) LOG.debug("Initiating driver migration for share %s.", share_ref['id']) @@ -780,6 +779,27 @@ class ShareManager(manager.SchedulerDependentManager): return True + def _cast_access_rules_to_readonly(self, context, src_share_instance, + share_server): + # Set all 'applying' or 'active' rules to 'queued_to_apply'. Since the + # share has a status set to 'migrating', this will render rules + # read/only. + acceptable_past_states = (constants.ACCESS_STATE_APPLYING, + constants.ACCESS_STATE_ACTIVE) + new_state = constants.ACCESS_STATE_QUEUED_TO_APPLY + conditionally_change = {k: new_state for k in acceptable_past_states} + self.access_helper.get_and_update_share_instance_access_rules( + context, share_instance_id=src_share_instance['id'], + conditionally_change=conditionally_change) + + self.access_helper.update_access_rules( + context, src_share_instance['id'], + share_server=share_server) + + utils.wait_for_access_update( + context, self.db, src_share_instance, + self.migration_wait_access_rules_timeout) + @periodic_task.periodic_task( spacing=CONF.migration_driver_continue_update_interval) @utils.require_driver_initialized @@ -932,17 +952,14 @@ class ShareManager(manager.SchedulerDependentManager): rpcapi = share_rpcapi.ShareAPI() - helper = migration.ShareMigrationHelper(context, self.db, share) + helper = migration.ShareMigrationHelper( + context, self.db, share, self.access_helper) share_server = self._get_share_server(context.elevated(), src_share_instance) - self.db.share_instance_update_access_status( - context, src_share_instance['id'], - constants.STATUS_OUT_OF_SYNC) - - self.access_helper.update_access_rules( - context, src_share_instance['id'], share_server=share_server) + self._cast_access_rules_to_readonly( + context, src_share_instance, share_server) try: dest_share_instance = helper.create_instance_and_wait( @@ -957,8 +974,7 @@ class ShareManager(manager.SchedulerDependentManager): msg = _("Failed to create instance on destination " "backend during migration of share %s.") % share['id'] LOG.exception(msg) - helper.cleanup_access_rules(src_share_instance, share_server, - self.driver) + helper.cleanup_access_rules(src_share_instance, share_server) raise exception.ShareMigrationFailed(reason=msg) ignore_list = self.driver.configuration.safe_get( @@ -987,8 +1003,7 @@ class ShareManager(manager.SchedulerDependentManager): "share %s.") % share['id'] LOG.exception(msg) helper.cleanup_new_instance(dest_share_instance) - helper.cleanup_access_rules(src_share_instance, share_server, - self.driver) + helper.cleanup_access_rules(src_share_instance, share_server) raise exception.ShareMigrationFailed(reason=msg) def _migration_complete_driver( @@ -1010,7 +1025,8 @@ class ShareManager(manager.SchedulerDependentManager): self.db.share_export_locations_update( context, dest_share_instance['id'], export_locations) - helper = migration.ShareMigrationHelper(context, self.db, share_ref) + helper = migration.ShareMigrationHelper( + context, self.db, share_ref, self.access_helper) helper.apply_new_access_rules(dest_share_instance) @@ -1032,15 +1048,11 @@ class ShareManager(manager.SchedulerDependentManager): self.db.share_instance_update( context, instance_id, {'status': constants.STATUS_INACTIVE}) - rules = self.db.share_access_get_all_for_instance( - context, instance_id) + rules = self.access_helper.get_and_update_share_instance_access_rules( + context, share_instance_id=instance_id) - for rule in rules: - access_mapping = self.db.share_instance_access_get( - context, rule['id'], instance_id) - - self.db.share_instance_access_delete( - context, access_mapping['id']) + self.access_helper.delete_share_instance_access_rules( + context, rules, instance_id) self.db.share_instance_delete(context, instance_id) LOG.info(_LI("Share instance %s: deleted successfully."), @@ -1113,7 +1125,8 @@ class ShareManager(manager.SchedulerDependentManager): share_server = self._get_share_server(context, src_share_instance) - helper = migration.ShareMigrationHelper(context, self.db, share_ref) + helper = migration.ShareMigrationHelper( + context, self.db, share_ref, self.access_helper) task_state = share_ref['task_state'] if task_state in (constants.TASK_STATE_DATA_COPYING_ERROR, @@ -1123,8 +1136,7 @@ class ShareManager(manager.SchedulerDependentManager): LOG.warning(msg) helper.cleanup_new_instance(dest_share_instance) - helper.cleanup_access_rules(src_share_instance, share_server, - self.driver) + helper.cleanup_access_rules(src_share_instance, share_server) if task_state == constants.TASK_STATE_DATA_COPYING_CANCELLED: self.db.share_instance_update( context, src_instance_id, @@ -1152,8 +1164,7 @@ class ShareManager(manager.SchedulerDependentManager): "of share %s.") % share_ref['id'] LOG.exception(msg) helper.cleanup_new_instance(dest_share_instance) - helper.cleanup_access_rules(src_share_instance, share_server, - self.driver) + helper.cleanup_access_rules(src_share_instance, share_server) raise exception.ShareMigrationFailed(reason=msg) self.db.share_update( @@ -1201,7 +1212,7 @@ class ShareManager(manager.SchedulerDependentManager): constants.TASK_STATE_DATA_COPYING_COMPLETED): helper = migration.ShareMigrationHelper( - context, self.db, share_ref) + context, self.db, share_ref, self.access_helper) self.db.share_instance_update( context, dest_share_instance['id'], @@ -1209,8 +1220,7 @@ class ShareManager(manager.SchedulerDependentManager): helper.cleanup_new_instance(dest_share_instance) - helper.cleanup_access_rules(src_share_instance, share_server, - self.driver) + helper.cleanup_access_rules(src_share_instance, share_server) else: self.driver.migration_cancel( context, src_share_instance, dest_share_instance, @@ -1382,9 +1392,8 @@ class ShareManager(manager.SchedulerDependentManager): def _update_share_replica_access_rules_state(self, context, share_replica_id, state): """Update the access_rules_status for the share replica.""" - - self.db.share_instance_update_access_status( - context, share_replica_id, state) + self.access_helper.get_and_update_share_instance_access_rules_status( + context, status=state, share_instance_id=share_replica_id) def _get_replica_snapshots_for_snapshot(self, context, snapshot_id, active_replica_id, @@ -1549,7 +1558,8 @@ class ShareManager(manager.SchedulerDependentManager): replica_ref.get('access_rules_status')) else: self._update_share_replica_access_rules_state( - context, share_replica['id'], constants.STATUS_ACTIVE) + context, share_replica['id'], + constants.STATUS_ACTIVE) LOG.info(_LI("Share replica %s created successfully."), share_replica['id']) @@ -1589,7 +1599,7 @@ class ShareManager(manager.SchedulerDependentManager): self.access_helper.update_access_rules( context, share_replica_id, - delete_rules="all", + delete_all_rules=True, share_server=share_server ) except Exception: @@ -2074,7 +2084,7 @@ class ShareManager(manager.SchedulerDependentManager): self.access_helper.update_access_rules( context, share_instance['id'], - delete_rules="all", + delete_all_rules=True, share_server=share_server ) except Exception as e: @@ -2222,7 +2232,7 @@ class ShareManager(manager.SchedulerDependentManager): self.access_helper.update_access_rules( context, share_instance_id, - delete_rules="all", + delete_all_rules=True, share_server=share_server ) except exception.ShareResourceNotFound: @@ -2711,42 +2721,18 @@ class ShareManager(manager.SchedulerDependentManager): @add_hooks @utils.require_driver_initialized - def allow_access(self, context, share_instance_id, access_rules): - """Allow access to some share instance.""" - share_instance = self._get_share_instance(context, share_instance_id) - status = share_instance['access_rules_status'] - - if status not in (constants.STATUS_UPDATING, - constants.STATUS_UPDATING_MULTIPLE, - constants.STATUS_ACTIVE): - add_rules = [self.db.share_access_get(context, rule_id) - for rule_id in access_rules] - - share_server = self._get_share_server(context, share_instance) - - return self.access_helper.update_access_rules( - context, - share_instance_id, - add_rules=add_rules, - share_server=share_server - ) - - @add_hooks - @utils.require_driver_initialized - def deny_access(self, context, share_instance_id, access_rules): - """Deny access to some share.""" - delete_rules = [self.db.share_access_get(context, rule_id) - for rule_id in access_rules] - + def update_access(self, context, share_instance_id): + """Allow/Deny access to some share.""" share_instance = self._get_share_instance(context, share_instance_id) share_server = self._get_share_server(context, share_instance) - return self.access_helper.update_access_rules( + LOG.debug("Received request to update access for share instance" + " %s." % share_instance_id) + + self.access_helper.update_access_rules( context, share_instance_id, - delete_rules=delete_rules, - share_server=share_server - ) + share_server=share_server) @periodic_task.periodic_task(spacing=CONF.periodic_interval) @utils.require_driver_initialized diff --git a/manila/share/migration.py b/manila/share/migration.py index 68ebddf930..0276e6727a 100644 --- a/manila/share/migration.py +++ b/manila/share/migration.py @@ -47,11 +47,12 @@ CONF.register_opts(migration_opts) class ShareMigrationHelper(object): - def __init__(self, context, db, share): + def __init__(self, context, db, share, access_helper): self.db = db self.share = share self.context = context + self.access_helper = access_helper self.api = share_api.API() self.migration_create_delete_share_timeout = ( @@ -131,7 +132,7 @@ class ShareMigrationHelper(object): LOG.warning(_LW("Failed to cleanup new instance during generic" " migration for share %s."), self.share['id']) - def cleanup_access_rules(self, share_instance, share_server, driver): + def cleanup_access_rules(self, share_instance, share_server): # NOTE(ganso): For the purpose of restoring the access rules, the share # instance status must not be "MIGRATING", else they would be cast to @@ -142,45 +143,36 @@ class ShareMigrationHelper(object): {'status': constants.STATUS_INACTIVE}) try: - self.revert_access_rules(share_instance, share_server, driver) + self.revert_access_rules(share_instance, share_server) except Exception: LOG.warning(_LW("Failed to cleanup access rules during generic" " migration for share %s."), self.share['id']) - def revert_access_rules(self, share_instance, share_server, driver): + def revert_access_rules(self, share_instance, share_server): - rules = self.db.share_access_get_all_for_instance( - self.context, share_instance['id']) + # Cast all rules to 'queued_to_apply' so that they can be re-applied. + updates = {'state': constants.ACCESS_STATE_QUEUED_TO_APPLY} + self.access_helper.get_and_update_share_instance_access_rules( + self.context, updates=updates, + share_instance_id=share_instance['id']) - if len(rules) > 0: - LOG.debug("Restoring all of share %s access rules according to " - "DB.", self.share['id']) + self.access_helper.update_access_rules( + self.context, share_instance['id'], share_server=share_server) - driver.update_access(self.context, share_instance, rules, - add_rules=[], delete_rules=[], - share_server=share_server) + utils.wait_for_access_update( + self.context, self.db, share_instance, + self.migration_wait_access_rules_timeout) def apply_new_access_rules(self, new_share_instance): self.db.share_instance_access_copy(self.context, self.share['id'], new_share_instance['id']) - rules = self.db.share_access_get_all_for_instance( - self.context, new_share_instance['id']) + self.api.allow_access_to_instance(self.context, new_share_instance) - if len(rules) > 0: - LOG.debug("Restoring all of share %s access rules according to " - "DB.", self.share['id']) - - # refresh share instance - new_share_instance = self.db.share_instance_get( - self.context, new_share_instance['id'], with_share_data=True) - - self.api.allow_access_to_instance(self.context, new_share_instance, - rules) - utils.wait_for_access_update( - self.context, self.db, new_share_instance, - self.migration_wait_access_rules_timeout) + utils.wait_for_access_update( + self.context, self.db, new_share_instance, + self.migration_wait_access_rules_timeout) @utils.retry(exception.ShareServerNotReady, retries=8) def wait_for_share_server(self, share_server_id): diff --git a/manila/share/rpcapi.py b/manila/share/rpcapi.py index bcddadc2f8..14a6e5d7ae 100644 --- a/manila/share/rpcapi.py +++ b/manila/share/rpcapi.py @@ -45,7 +45,8 @@ class ShareAPI(object): migrate_share() get_migration_info() get_driver_migration_info() - 1.7 - Update target call API in allow/deny access methods + 1.7 - Update target call API in allow/deny access methods (Removed + in 1.14) 1.8 - Introduce Share Replication: create_share_replica() delete_share_replica() @@ -65,6 +66,7 @@ class ShareAPI(object): migration_get_progress method signature, rename migration_get_info() to connection_get_info() 1.13 - Introduce share revert to snapshot: revert_to_snapshot() + 1.14 - Add update_access() and remove allow_access() and deny_access(). """ BASE_RPC_API_VERSION = '1.0' @@ -73,7 +75,7 @@ class ShareAPI(object): super(ShareAPI, self).__init__() target = messaging.Target(topic=CONF.share_topic, version=self.BASE_RPC_API_VERSION) - self.client = rpc.get_client(target, version_cap='1.13') + self.client = rpc.get_client(target, version_cap='1.14') def create_share_instance(self, context, share_instance, host, request_spec, filter_properties, @@ -200,28 +202,11 @@ class ShareAPI(object): share_id=share_id, force=force) - @staticmethod - def _get_access_rules(access): - if isinstance(access, list): - return [rule['id'] for rule in access] - else: - return [access['id']] - - def allow_access(self, context, share_instance, access): + def update_access(self, context, share_instance): host = utils.extract_host(share_instance['host']) - call_context = self.client.prepare(server=host, version='1.7') - call_context.cast(context, - 'allow_access', - share_instance_id=share_instance['id'], - access_rules=self._get_access_rules(access)) - - def deny_access(self, context, share_instance, access): - host = utils.extract_host(share_instance['host']) - call_context = self.client.prepare(server=host, version='1.7') - call_context.cast(context, - 'deny_access', - share_instance_id=share_instance['id'], - access_rules=self._get_access_rules(access)) + call_context = self.client.prepare(server=host, version='1.14') + call_context.cast(context, 'update_access', + share_instance_id=share_instance['id']) def publish_service_capabilities(self, context): call_context = self.client.prepare(fanout=True, version='1.0') diff --git a/manila/tests/api/contrib/stubs.py b/manila/tests/api/contrib/stubs.py index 9a71dfbbc3..aab6985bd8 100644 --- a/manila/tests/api/contrib/stubs.py +++ b/manila/tests/api/contrib/stubs.py @@ -52,6 +52,7 @@ def stub_share(id, **kwargs): 'availability_zone': 'fakeaz', 'share_network_id': None, 'share_server_id': 'fake_share_server_id', + 'access_rules_status': 'active', } if 'instance' in kwargs: share_instance.update(kwargs.pop('instance')) @@ -92,6 +93,8 @@ def stub_share(id, **kwargs): else: fake_share.is_busy = False + fake_share.instances = [fake_share.instance] + return fake_share diff --git a/manila/tests/api/v1/test_shares.py b/manila/tests/api/v1/test_shares.py index f46d73f6ac..d26fc61df8 100644 --- a/manila/tests/api/v1/test_shares.py +++ b/manila/tests/api/v1/test_shares.py @@ -30,6 +30,7 @@ from manila.common import constants from manila import context from manila import db from manila import exception +from manila import policy from manila.share import api as share_api from manila.share import share_types from manila import test @@ -862,6 +863,7 @@ class ShareActionsTest(test.TestCase): super(self.__class__, self).setUp() self.controller = shares.ShareController() self.mock_object(share_api.API, 'get', stubs.stub_share_get) + self.mock_policy_check = self.mock_object(policy, 'check_policy') @ddt.data( {'access_type': 'ip', 'access_to': '127.0.0.1'}, @@ -885,8 +887,12 @@ class ShareActionsTest(test.TestCase): body = {'os-allow_access': access} expected = {'access': {'fake': 'fake'}} req = fakes.HTTPRequest.blank('/v1/tenant1/shares/%s/action' % id) + res = self.controller._allow_access(req, id, body) + self.assertEqual(expected, res) + self.mock_policy_check.assert_called_once_with( + req.environ['manila.context'], 'share', 'allow_access') @ddt.data( {'access_type': 'error_type', 'access_to': '127.0.0.1'}, @@ -907,8 +913,11 @@ class ShareActionsTest(test.TestCase): id = 'fake_share_id' body = {'os-allow_access': access} req = fakes.HTTPRequest.blank('/v1/tenant1/shares/%s/action' % id) + self.assertRaises(webob.exc.HTTPBadRequest, self.controller._allow_access, req, id, body) + self.mock_policy_check.assert_called_once_with( + req.environ['manila.context'], 'share', 'allow_access') def test_deny_access(self): def _stub_deny_access(*args, **kwargs): @@ -920,8 +929,12 @@ class ShareActionsTest(test.TestCase): id = 'fake_share_id' body = {"os-deny_access": {"access_id": 'fake_acces_id'}} req = fakes.HTTPRequest.blank('/v1/tenant1/shares/%s/action' % id) + res = self.controller._deny_access(req, id, body) + self.assertEqual(202, res.status_int) + self.mock_policy_check.assert_called_once_with( + req.environ['manila.context'], 'share', 'deny_access') def test_deny_access_not_found(self): def _stub_deny_access(*args, **kwargs): @@ -933,11 +946,29 @@ class ShareActionsTest(test.TestCase): id = 'super_fake_share_id' body = {"os-deny_access": {"access_id": 'fake_acces_id'}} req = fakes.HTTPRequest.blank('/v1/tenant1/shares/%s/action' % id) + self.assertRaises(webob.exc.HTTPNotFound, self.controller._deny_access, req, id, body) + self.mock_policy_check.assert_called_once_with( + req.environ['manila.context'], 'share', 'deny_access') + + @ddt.data('_allow_access', '_deny_access') + def test_allow_access_deny_access_policy_not_authorized(self, method): + req = fakes.HTTPRequest.blank('/v1/tenant1/shares/someuuid/action') + action = method[1:] + body = {action: None} + noauthexc = exception.PolicyNotAuthorized(action=action) + with mock.patch.object( + policy, 'check_policy', mock.Mock(side_effect=noauthexc)): + method = getattr(self.controller, method) + + self.assertRaises( + webob.exc.HTTPForbidden, method, req, body, 'someuuid') + policy.check_policy.assert_called_once_with( + req.environ['manila.context'], 'share', action) def test_access_list(self): fake_access_list = [ diff --git a/manila/tests/api/v2/test_shares.py b/manila/tests/api/v2/test_shares.py index 5b302fe485..42fc3c5df4 100644 --- a/manila/tests/api/v2/test_shares.py +++ b/manila/tests/api/v2/test_shares.py @@ -15,6 +15,7 @@ import copy import datetime +import itertools import ddt import mock @@ -1174,6 +1175,24 @@ class ShareAPITest(test.TestCase): self.assertEqual(expected, res_dict) + @ddt.data(('2.10', True), ('2.27', True), ('2.28', False)) + @ddt.unpack + def test_share_show_access_rules_status_translated(self, version, + translated): + share = db_utils.create_share( + access_rules_status=constants.SHARE_INSTANCE_RULES_SYNCING, + status=constants.STATUS_AVAILABLE) + self.mock_object(share_api.API, 'get', mock.Mock(return_value=share)) + req = fakes.HTTPRequest.blank( + '/shares/%s' % share['id'], version=version) + + res_dict = self.controller.show(req, share['id']) + + expected = (constants.STATUS_OUT_OF_SYNC if translated else + constants.SHARE_INSTANCE_RULES_SYNCING) + + self.assertEqual(expected, res_dict['share']['access_rules_status']) + def test_share_delete(self): req = fakes.HTTPRequest.blank('/shares/1') resp = self.controller.delete(req, 1) @@ -1626,13 +1645,14 @@ class ShareActionsTest(test.TestCase): self.mock_object(self.controller._access_view_builder, 'view', mock.Mock(return_value={'access': {'fake': 'fake'}})) - id = 'fake_share_id' body = {'allow_access': access} expected = {'access': {'fake': 'fake'}} req = fakes.HTTPRequest.blank( '/v2/tenant1/shares/%s/action' % id, version="2.7") + res = self.controller.allow_access(req, id, body) + self.assertEqual(expected, res) @ddt.data( @@ -1690,6 +1710,124 @@ class ShareActionsTest(test.TestCase): res = self.controller.allow_access(req, id, body) self.assertEqual(expected, res) + @ddt.data('2.1', '2.27') + def test_allow_access_access_rules_status_is_in_error(self, version): + share = db_utils.create_share( + access_rules_status=constants.SHARE_INSTANCE_RULES_ERROR) + + req = fakes.HTTPRequest.blank( + '/v2/shares/%s/action' % share['id'], version=version) + self.mock_object(share_api.API, 'get', mock.Mock(return_value=share)) + self.mock_object(share_api.API, 'allow_access') + if (api_version.APIVersionRequest(version) >= + api_version.APIVersionRequest('2.7')): + key = 'allow_access' + method = self.controller.allow_access + else: + key = 'os-allow_access' + method = self.controller.allow_access_legacy + + body = { + key: { + 'access_type': 'user', + 'access_to': 'crimsontide', + 'access_level': 'rw', + } + } + + self.assertRaises(webob.exc.HTTPBadRequest, + method, req, share['id'], body) + self.assertFalse(share_api.API.allow_access.called) + + @ddt.data(*itertools.product( + ('2.1', '2.27'), (constants.SHARE_INSTANCE_RULES_SYNCING, + constants.STATUS_ACTIVE))) + @ddt.unpack + def test_allow_access_no_transitional_states(self, version, status): + share = db_utils.create_share(access_rules_status=status, + status=constants.STATUS_AVAILABLE) + req = fakes.HTTPRequest.blank( + '/v2/shares/%s/action' % share['id'], version=version) + ctxt = req.environ['manila.context'] + access = { + 'access_type': 'user', + 'access_to': 'clemsontigers', + 'access_level': 'rw', + } + expected_mapping = { + constants.SHARE_INSTANCE_RULES_SYNCING: constants.STATUS_NEW, + constants.SHARE_INSTANCE_RULES_ERROR: + constants.ACCESS_STATE_ERROR, + constants.STATUS_ACTIVE: constants.ACCESS_STATE_ACTIVE, + } + share = db.share_get(ctxt, share['id']) + updated_access = db_utils.create_access(share_id=share['id'], **access) + expected_access = access + expected_access.update( + { + 'id': updated_access['id'], + 'state': expected_mapping[share['access_rules_status']], + 'share_id': updated_access['share_id'], + }) + + if (api_version.APIVersionRequest(version) >= + api_version.APIVersionRequest('2.7')): + key = 'allow_access' + method = self.controller.allow_access + else: + key = 'os-allow_access' + method = self.controller.allow_access_legacy + if (api_version.APIVersionRequest(version) >= + api_version.APIVersionRequest('2.13')): + expected_access['access_key'] = None + self.mock_object(share_api.API, 'get', mock.Mock(return_value=share)) + self.mock_object(share_api.API, 'allow_access', + mock.Mock(return_value=updated_access)) + body = {key: access} + + access = method(req, share['id'], body) + + self.assertEqual(expected_access, access['access']) + share_api.API.allow_access.assert_called_once_with( + req.environ['manila.context'], share, 'user', + 'clemsontigers', 'rw') + + @ddt.data(*itertools.product( + set(['2.28', api_version._MAX_API_VERSION]), + (constants.SHARE_INSTANCE_RULES_ERROR, + constants.SHARE_INSTANCE_RULES_SYNCING, constants.STATUS_ACTIVE))) + @ddt.unpack + def test_allow_access_access_rules_status_dont_care(self, version, status): + access = { + 'access_type': 'user', + 'access_to': 'clemsontigers', + 'access_level': 'rw', + } + updated_access = db_utils.create_access(**access) + expected_access = access + expected_access.update( + { + 'id': updated_access['id'], + 'state': updated_access['state'], + 'share_id': updated_access['share_id'], + 'access_key': None, + }) + + share = db_utils.create_share(access_rules_status=status) + req = fakes.HTTPRequest.blank( + '/v2/shares/%s/action' % share['id'], version=version) + self.mock_object(share_api.API, 'get', mock.Mock(return_value=share)) + self.mock_object(share_api.API, 'allow_access', + mock.Mock(return_value=updated_access)) + body = {'allow_access': access} + + access = self.controller.allow_access(req, share['id'], body) + + self.assertEqual(expected_access, access['access']) + share_api.API.allow_access.assert_called_once_with( + req.environ['manila.context'], share, 'user', + 'clemsontigers', 'rw') + def test_deny_access(self): def _stub_deny_access(*args, **kwargs): pass diff --git a/manila/tests/api/views/test_share_accesses.py b/manila/tests/api/views/test_share_accesses.py index 85a18c5eec..a86ad5cca8 100644 --- a/manila/tests/api/views/test_share_accesses.py +++ b/manila/tests/api/views/test_share_accesses.py @@ -14,9 +14,11 @@ # under the License. import ddt +import mock from manila.api.openstack import api_version_request as api_version from manila.api.views import share_accesses +from manila.share import api from manila import test from manila.tests.api import fakes @@ -36,6 +38,9 @@ class ViewBuilderTestCase(test.TestCase): 'state': 'fakeaccessstate', 'access_key': 'fakeaccesskey', } + self.fake_share = { + 'access_rules_status': self.fake_access['state'], + } def test_collection_name(self): self.assertEqual('share_accesses', self.builder._collection_name) @@ -43,6 +48,8 @@ class ViewBuilderTestCase(test.TestCase): @ddt.data("2.20", "2.21") def test_view(self, version): req = fakes.HTTPRequest.blank('/shares', version=version) + self.mock_object(api.API, 'get', + mock.Mock(return_value=self.fake_share)) result = self.builder.view(req, self.fake_access) @@ -55,6 +62,8 @@ class ViewBuilderTestCase(test.TestCase): @ddt.data("2.20", "2.21") def test_summary_view(self, version): req = fakes.HTTPRequest.blank('/shares', version=version) + self.mock_object(api.API, 'get', + mock.Mock(return_value=self.fake_share)) result = self.builder.summary_view(req, self.fake_access) @@ -68,6 +77,8 @@ class ViewBuilderTestCase(test.TestCase): @ddt.data("2.20", "2.21") def test_list_view(self, version): req = fakes.HTTPRequest.blank('/shares', version=version) + self.mock_object(api.API, 'get', + mock.Mock(return_value=self.fake_share)) accesses = [self.fake_access, ] result = self.builder.list_view(req, accesses) diff --git a/manila/tests/data/test_helper.py b/manila/tests/data/test_helper.py index 0bfca026f9..0f44e0dc31 100644 --- a/manila/tests/data/test_helper.py +++ b/manila/tests/data/test_helper.py @@ -82,7 +82,8 @@ class DataServiceHelperTestCase(test.TestCase): self.mock_object( self.helper.db, 'share_access_get_all_by_type_and_access', mock.Mock(return_value=[access])) - self.mock_object(self.helper, '_change_data_access_to_instance') + change_data_access_call = self.mock_object( + self.helper, '_change_data_access_to_instance') self.mock_object(self.helper.db, 'share_instance_access_create', mock.Mock(return_value=access)) @@ -110,14 +111,14 @@ class DataServiceHelperTestCase(test.TestCase): self.helper.db.share_instance_access_create.assert_has_calls( access_create_calls) change_access_calls = [ - mock.call(self.share_instance, access, allow=False), - mock.call(self.share_instance, access, allow=True), + mock.call(self.share_instance, [access], deny=True), ] if allow_dest_instance: change_access_calls.append( - mock.call(self.share_instance, access, allow=True)) - self.helper._change_data_access_to_instance.assert_has_calls( - change_access_calls) + mock.call(self.share_instance, access)) + self.assertEqual(len(change_access_calls), + change_data_access_call.call_count) + change_data_access_call.assert_has_calls(change_access_calls) @ddt.data({'ip': []}, {'cert': []}, {'user': []}, {'cephx': []}, {'x': []}) def test__get_access_entries_according_to_mapping(self, mapping): @@ -154,9 +155,8 @@ class DataServiceHelperTestCase(test.TestCase): [self.access], self.share_instance['id']) # asserts - self.helper._change_data_access_to_instance.\ - assert_called_once_with( - self.share_instance['id'], self.access, allow=False) + self.helper._change_data_access_to_instance.assert_called_once_with( + self.share_instance['id'], [self.access], deny=True) @ddt.data(None, Exception('fake')) def test_cleanup_data_access(self, exc): @@ -225,34 +225,39 @@ class DataServiceHelperTestCase(test.TestCase): self.assertTrue(data_copy_helper.LOG.warning.called) @ddt.data(True, False) - def test__change_data_access_to_instance(self, allow): + def test__change_data_access_to_instance(self, deny): + access_rule = db_utils.create_access(share_id=self.share['id']) + access_rule = db.share_instance_access_get( + self.context, access_rule['id'], self.share_instance['id']) # mocks - self.mock_object(self.helper.db, 'share_instance_update_access_status') - - if allow: - self.mock_object(share_rpc.ShareAPI, 'allow_access') - else: - self.mock_object(share_rpc.ShareAPI, 'deny_access') - + self.mock_object(share_rpc.ShareAPI, 'update_access') self.mock_object(utils, 'wait_for_access_update') + mock_access_rules_status_update = self.mock_object( + self.helper.access_helper, + 'get_and_update_share_instance_access_rules_status') + mock_rules_update = self.mock_object( + self.helper.access_helper, + 'get_and_update_share_instance_access_rules') # run self.helper._change_data_access_to_instance( - self.share_instance, self.access, allow=allow) + self.share_instance, access_rule, deny=deny) # asserts - self.helper.db.share_instance_update_access_status.\ - assert_called_once_with(self.context, self.share_instance['id'], - constants.STATUS_OUT_OF_SYNC) + if deny: + mock_rules_update.assert_called_once_with( + self.context, share_instance_id=self.share_instance['id'], + filters={'access_id': [access_rule['id']]}, + updates={'state': constants.ACCESS_STATE_QUEUED_TO_DENY}) - if allow: - share_rpc.ShareAPI.allow_access.assert_called_once_with( - self.context, self.share_instance, self.access) else: - share_rpc.ShareAPI.deny_access.assert_called_once_with( - self.context, self.share_instance, self.access) - + self.assertFalse(mock_rules_update.called) + share_rpc.ShareAPI.update_access.assert_called_once_with( + self.context, self.share_instance) + mock_access_rules_status_update.assert_called_once_with( + self.context, status=constants.SHARE_INSTANCE_RULES_SYNCING, + share_instance_id=self.share_instance['id']) utils.wait_for_access_update.assert_called_once_with( self.context, self.helper.db, self.share_instance, data_copy_helper.CONF.data_access_wait_access_rules_timeout) diff --git a/manila/tests/db/migrations/alembic/migrations_data_checks.py b/manila/tests/db/migrations/alembic/migrations_data_checks.py index de94669bb3..f1827d761b 100644 --- a/manila/tests/db/migrations/alembic/migrations_data_checks.py +++ b/manila/tests/db/migrations/alembic/migrations_data_checks.py @@ -1580,3 +1580,139 @@ class RemoveNovaNetIdColumnFromShareNetworks(BaseMigrationChecks): for row in rows: self.test_case.assertTrue(hasattr(row, self.nova_net_column_name)) self.test_case.assertIsNone(row[self.nova_net_column_name]) + + +@map_to_migration('54667b9cade7') +class RestoreStateToShareInstanceAccessMap(BaseMigrationChecks): + new_instance_mapping_state = { + constants.STATUS_ACTIVE: constants.STATUS_ACTIVE, + constants.SHARE_INSTANCE_RULES_SYNCING: + constants.ACCESS_STATE_QUEUED_TO_APPLY, + constants.STATUS_OUT_OF_SYNC: constants.ACCESS_STATE_QUEUED_TO_APPLY, + 'updating': constants.ACCESS_STATE_QUEUED_TO_APPLY, + 'updating_multiple': constants.ACCESS_STATE_QUEUED_TO_APPLY, + constants.SHARE_INSTANCE_RULES_ERROR: constants.ACCESS_STATE_ERROR, + } + + new_access_rules_status = { + constants.STATUS_ACTIVE: constants.STATUS_ACTIVE, + constants.STATUS_OUT_OF_SYNC: constants.SHARE_INSTANCE_RULES_SYNCING, + 'updating': constants.SHARE_INSTANCE_RULES_SYNCING, + 'updating_multiple': constants.SHARE_INSTANCE_RULES_SYNCING, + constants.SHARE_INSTANCE_RULES_ERROR: + constants.SHARE_INSTANCE_RULES_ERROR, + } + + @staticmethod + def generate_share_instance(sid, access_rules_status): + share_instance_data = { + 'id': uuidutils.generate_uuid(), + 'deleted': 'False', + 'host': 'fake', + 'share_id': sid, + 'status': constants.STATUS_AVAILABLE, + 'access_rules_status': access_rules_status + } + return share_instance_data + + @staticmethod + def generate_share_instance_access_map(share_access_data_id, + share_instance_id): + share_instance_access_data = { + 'id': uuidutils.generate_uuid(), + 'share_instance_id': share_instance_id, + 'access_id': share_access_data_id, + 'deleted': 'False' + } + return share_instance_access_data + + def setup_upgrade_data(self, engine): + share_data = { + 'id': uuidutils.generate_uuid(), + 'share_proto': 'fake', + '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_instances = [ + self.generate_share_instance( + share_data['id'], constants.STATUS_ACTIVE), + self.generate_share_instance( + share_data['id'], constants.STATUS_OUT_OF_SYNC), + self.generate_share_instance( + share_data['id'], constants.STATUS_ERROR), + self.generate_share_instance( + share_data['id'], 'updating'), + self.generate_share_instance( + share_data['id'], 'updating_multiple'), + ] + self.updating_share_instance = share_instances[3] + self.updating_multiple_share_instance = share_instances[4] + + share_instance_table = utils.load_table('share_instances', engine) + for share_instance_data in share_instances: + engine.execute(share_instance_table.insert(share_instance_data)) + + share_access_data = { + 'id': uuidutils.generate_uuid(), + 'share_id': share_data['id'], + 'access_type': 'fake', + 'access_to': 'alice', + 'deleted': 'False' + } + share_access_table = utils.load_table('share_access_map', engine) + engine.execute(share_access_table.insert(share_access_data)) + + share_instance_access_data = [] + for share_instance in share_instances: + sia_map = self.generate_share_instance_access_map( + share_access_data['id'], share_instance['id']) + share_instance_access_data.append(sia_map) + + share_instance_access_table = utils.load_table( + 'share_instance_access_map', engine) + for sia_map in share_instance_access_data: + engine.execute(share_instance_access_table.insert(sia_map)) + + def check_upgrade(self, engine, data): + share_instance_table = utils.load_table('share_instances', engine) + sia_table = utils.load_table('share_instance_access_map', engine) + + for rule in engine.execute(sia_table.select()): + self.test_case.assertTrue(hasattr(rule, 'state')) + correlated_share_instances = engine.execute( + share_instance_table.select().where( + share_instance_table.c.id == rule['share_instance_id'])) + access_rules_status = getattr(correlated_share_instances.first(), + 'access_rules_status') + self.test_case.assertEqual( + self.new_instance_mapping_state[access_rules_status], + rule['state']) + + for instance in engine.execute(share_instance_table.select()): + self.test_case.assertTrue(instance['access_rules_status'] + not in ('updating', + 'updating_multiple', + constants.STATUS_OUT_OF_SYNC)) + if instance['id'] in (self.updating_share_instance['id'], + self.updating_multiple_share_instance['id']): + self.test_case.assertEqual( + constants.SHARE_INSTANCE_RULES_SYNCING, + instance['access_rules_status']) + + def check_downgrade(self, engine): + share_instance_table = utils.load_table('share_instances', engine) + sia_table = utils.load_table('share_instance_access_map', engine) + for rule in engine.execute(sia_table.select()): + self.test_case.assertFalse(hasattr(rule, 'state')) + + for instance in engine.execute(share_instance_table.select()): + if instance['id'] in (self.updating_share_instance['id'], + self.updating_multiple_share_instance['id']): + self.test_case.assertEqual( + constants.STATUS_OUT_OF_SYNC, + instance['access_rules_status']) diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index 1671ab91ec..c71e2aacec 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -85,7 +85,6 @@ class GenericDatabaseAPITestCase(test.TestCase): db_api.share_instance_access_delete( self.ctxt, share_access.instance_mappings[0].id) - db_api.share_access_delete(self.ctxt, share_access.id) self.assertRaises(exception.NotFound, db_api.share_access_get, self.ctxt, share_access.id) @@ -98,44 +97,126 @@ class ShareAccessDatabaseAPITestCase(test.TestCase): super(ShareAccessDatabaseAPITestCase, self).setUp() self.ctxt = context.get_admin_context() - def test_share_instance_update_access_status(self): + @ddt.data(0, 3) + def test_share_access_get_all_for_share(self, len_rules): share = db_utils.create_share() - share_instance = db_utils.create_share_instance(share_id=share['id']) - db_utils.create_access(share_id=share_instance['share_id']) + rules = [db_utils.create_access(share_id=share['id']) + for i in range(0, len_rules)] + rule_ids = [r['id'] for r in rules] - db_api.share_instance_update_access_status( - self.ctxt, - share_instance['id'], - constants.STATUS_ACTIVE - ) + result = db_api.share_access_get_all_for_share(self.ctxt, share['id']) - result = db_api.share_instance_get(self.ctxt, share_instance['id']) + self.assertEqual(len_rules, len(result)) + result_ids = [r['id'] for r in result] + self.assertEqual(rule_ids, result_ids) - self.assertEqual(constants.STATUS_ACTIVE, - result['access_rules_status']) - - def test_share_instance_update_access_status_invalid(self): + def test_share_access_get_all_for_share_no_instance_mappings(self): share = db_utils.create_share() - share_instance = db_utils.create_share_instance(share_id=share['id']) - db_utils.create_access(share_id=share_instance['share_id']) + share_instance = share['instance'] + rule = db_utils.create_access(share_id=share['id']) + # Mark instance mapping soft deleted + db_api.share_instance_access_update( + self.ctxt, rule['id'], share_instance['id'], {'deleted': "True"}) - self.assertRaises( - db_exception.DBError, - db_api.share_instance_update_access_status, - self.ctxt, share_instance['id'], - "fake_status" - ) + result = db_api.share_access_get_all_for_share(self.ctxt, share['id']) - @ddt.data(None, 'rhubarb') - def test_share_access_update_access_key(self, key_value): + self.assertEqual([], result) + + def test_share_instance_access_update(self): 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) + instance_access_mapping = db_api.share_instance_access_get( + self.ctxt, access['id'], share.instance['id']) + self.assertEqual(constants.ACCESS_STATE_QUEUED_TO_APPLY, + access['state']) + self.assertIsNone(access['access_key']) + db_api.share_instance_access_update( + self.ctxt, access['id'], share.instance['id'], + {'state': constants.STATUS_ERROR, 'access_key': 'watson4heisman'}) + + instance_access_mapping = db_api.share_instance_access_get( + self.ctxt, access['id'], share.instance['id']) access = db_api.share_access_get(self.ctxt, access['id']) - self.assertEqual(key_value, access['access_key']) + self.assertEqual(constants.STATUS_ERROR, + instance_access_mapping['state']) + self.assertEqual('watson4heisman', access['access_key']) + + @ddt.data(True, False) + def test_share_access_get_all_for_instance_with_share_access_data( + self, with_share_access_data): + share = db_utils.create_share() + access_1 = db_utils.create_access(share_id=share['id']) + access_2 = db_utils.create_access(share_id=share['id']) + share_access_keys = ('access_to', 'access_type', 'access_level', + 'share_id') + + rules = db_api.share_access_get_all_for_instance( + self.ctxt, share.instance['id'], + with_share_access_data=with_share_access_data) + + share_access_keys_present = True if with_share_access_data else False + actual_access_ids = [r['access_id'] for r in rules] + self.assertEqual(sorted([access_1['id'], access_2['id']]), + sorted(actual_access_ids)) + for rule in rules: + for key in share_access_keys: + self.assertEqual(share_access_keys_present, key in rule) + self.assertTrue('state' in rule) + + def test_share_access_get_all_for_instance_with_filters(self): + share = db_utils.create_share() + new_share_instance = db_utils.create_share_instance( + share_id=share['id']) + access_1 = db_utils.create_access(share_id=share['id']) + access_2 = db_utils.create_access(share_id=share['id']) + share_access_keys = ('access_to', 'access_type', 'access_level', + 'share_id') + db_api.share_instance_access_update( + self.ctxt, access_1['id'], new_share_instance['id'], + {'state': constants.STATUS_ACTIVE}) + + rules = db_api.share_access_get_all_for_instance( + self.ctxt, new_share_instance['id'], + filters={'state': constants.ACCESS_STATE_QUEUED_TO_APPLY}) + + self.assertEqual(1, len(rules)) + self.assertEqual(access_2['id'], rules[0]['access_id']) + + for rule in rules: + for key in share_access_keys: + self.assertTrue(key in rule) + + def test_share_instance_access_delete(self): + share = db_utils.create_share() + access = db_utils.create_access(share_id=share['id']) + instance_access_mapping = db_api.share_instance_access_get( + self.ctxt, access['id'], share.instance['id']) + + db_api.share_instance_access_delete( + self.ctxt, instance_access_mapping['id']) + + rules = db_api.share_access_get_all_for_instance( + self.ctxt, share.instance['id']) + self.assertEqual([], rules) + + self.assertRaises(exception.NotFound, db_api.share_instance_access_get, + self.ctxt, access['id'], share['instance']['id']) + + @ddt.data(True, False) + def test_share_instance_access_get_with_share_access_data( + self, with_share_access_data): + share = db_utils.create_share() + access = db_utils.create_access(share_id=share['id']) + + instance_access = db_api.share_instance_access_get( + self.ctxt, access['id'], share['instance']['id'], + with_share_access_data=with_share_access_data) + + for key in ('share_id', 'access_type', 'access_to', 'access_level', + 'access_key'): + self.assertEqual(with_share_access_data, key in instance_access) @ddt.ddt diff --git a/manila/tests/db/sqlalchemy/test_models.py b/manila/tests/db/sqlalchemy/test_models.py index 6f75faa2f5..06b456dac0 100644 --- a/manila/tests/db/sqlalchemy/test_models.py +++ b/manila/tests/db/sqlalchemy/test_models.py @@ -17,6 +17,8 @@ import ddt from manila.common import constants +from manila import context +from manila.db.sqlalchemy import api as db_api from manila import test from manila.tests import db_utils @@ -152,8 +154,8 @@ class ShareTestCase(test.TestCase): self.assertEqual(constants.STATUS_ACTIVE, share.access_rules_status) - @ddt.data(constants.STATUS_ACTIVE, constants.STATUS_OUT_OF_SYNC, - constants.STATUS_ERROR) + @ddt.data(constants.STATUS_ACTIVE, constants.SHARE_INSTANCE_RULES_SYNCING, + constants.SHARE_INSTANCE_RULES_ERROR) def test_access_rules_status(self, access_status): instances = [ db_utils.create_share_instance( @@ -173,6 +175,43 @@ class ShareTestCase(test.TestCase): @ddt.ddt +class ShareAccessTestCase(test.TestCase): + """Testing of SQLAlchemy Share Access related model classes.""" + + @ddt.data(constants.ACCESS_STATE_QUEUED_TO_APPLY, + constants.ACCESS_STATE_ACTIVE, constants.ACCESS_STATE_ERROR, + constants.ACCESS_STATE_APPLYING) + def test_share_access_mapping_state(self, expected_status): + ctxt = context.get_admin_context() + + share = db_utils.create_share() + share_instances = [ + share.instance, + db_utils.create_share_instance(share_id=share['id']), + db_utils.create_share_instance(share_id=share['id']), + db_utils.create_share_instance(share_id=share['id']), + ] + access_rule = db_utils.create_access(share_id=share['id']) + + # Update the access mapping states + db_api.share_instance_access_update( + ctxt, access_rule['id'], share_instances[0]['id'], + {'state': constants.ACCESS_STATE_ACTIVE}) + db_api.share_instance_access_update( + ctxt, access_rule['id'], share_instances[1]['id'], + {'state': expected_status}) + db_api.share_instance_access_update( + ctxt, access_rule['id'], share_instances[2]['id'], + {'state': constants.ACCESS_STATE_ACTIVE}) + db_api.share_instance_access_update( + ctxt, access_rule['id'], share_instances[3]['id'], + {'deleted': 'True', 'state': constants.STATUS_DELETED}) + + access_rule = db_api.share_access_get(ctxt, access_rule['id']) + + self.assertEqual(expected_status, access_rule['state']) + + class ShareSnapshotTestCase(test.TestCase): """Testing of SQLAlchemy ShareSnapshot model class.""" diff --git a/manila/tests/db_utils.py b/manila/tests/db_utils.py index d16f1c80df..f160ca17de 100644 --- a/manila/tests/db_utils.py +++ b/manila/tests/db_utils.py @@ -168,12 +168,21 @@ def create_snapshot_instance(snapshot_id, **kwargs): def create_access(**kwargs): """Create a access rule object.""" + state = kwargs.pop('state', constants.ACCESS_STATE_QUEUED_TO_APPLY) access = { 'access_type': 'fake_type', 'access_to': 'fake_IP', - 'share_id': None, + 'share_id': kwargs.pop('share_id', None) or create_share()['id'], } - return _create_db_row(db.share_access_create, access, kwargs) + access.update(kwargs) + share_access_rule = _create_db_row(db.share_access_create, access, kwargs) + + for mapping in share_access_rule.instance_mappings: + db.share_instance_access_update( + context.get_admin_context(), share_access_rule['id'], + mapping.share_instance_id, {'state': state}) + + return share_access_rule def create_share_server(**kwargs): diff --git a/manila/tests/fake_share.py b/manila/tests/fake_share.py index 0f0f19878f..82e5ab26fb 100644 --- a/manila/tests/fake_share.py +++ b/manila/tests/fake_share.py @@ -242,7 +242,7 @@ def fake_replica(id=None, as_primitive=True, for_manager=False, **kwargs): 'export_locations': [{'path': 'path1'}, {'path': 'path2'}], 'share_network_id': '4ccd5318-65f1-11e5-9d70-feff819cdc9f', 'share_server_id': '53099868-65f1-11e5-9d70-feff819cdc9f', - 'access_rules_status': 'out_of_sync', + 'access_rules_status': constants.SHARE_INSTANCE_RULES_SYNCING, } if for_manager: replica.update({ diff --git a/manila/tests/share/drivers/cephfs/test_cephfs_native.py b/manila/tests/share/drivers/cephfs/test_cephfs_native.py index 3756cdb4fc..04a1e6eb91 100644 --- a/manila/tests/share/drivers/cephfs/test_cephfs_native.py +++ b/manila/tests/share/drivers/cephfs/test_cephfs_native.py @@ -249,24 +249,26 @@ class CephFSNativeDriverTestCase(test.TestCase): def test_update_access_add_rm(self): alice = { - 'id': 'accessid1', + 'id': 'instance_mapping_id1', + 'access_id': 'accessid1', 'access_level': 'rw', 'access_type': 'cephx', 'access_to': 'alice' } bob = { - 'id': 'accessid2', + 'id': 'instance_mapping_id2', + 'access_id': 'accessid2', 'access_level': 'rw', 'access_type': 'cephx', 'access_to': 'bob' } - access_keys = self._driver.update_access(self._context, self._share, - access_rules=[alice], - add_rules=[alice], - delete_rules=[bob]) + access_updates = self._driver.update_access( + self._context, self._share, access_rules=[alice], + add_rules=[alice], delete_rules=[bob]) - self.assertEqual({'accessid1': 'abc123'}, access_keys) + self.assertEqual( + {'accessid1': {'access_key': 'abc123'}}, access_updates) self._driver._volume_client.authorize.assert_called_once_with( self._driver._share_path(self._share), "alice", @@ -279,19 +281,21 @@ class CephFSNativeDriverTestCase(test.TestCase): @ddt.data(None, 1) def test_update_access_all(self, volume_client_version): alice = { - 'id': 'accessid1', + 'id': 'instance_mapping_id1', + 'access_id': 'accessid1', 'access_level': 'rw', 'access_type': 'cephx', 'access_to': 'alice' } self._driver.volume_client.version = volume_client_version - access_keys = self._driver.update_access(self._context, self._share, - access_rules=[alice], - add_rules=[], - delete_rules=[]) + access_updates = self._driver.update_access(self._context, self._share, + access_rules=[alice], + add_rules=[], + delete_rules=[]) - self.assertEqual({'accessid1': 'abc123'}, access_keys) + self.assertEqual( + {'accessid1': {'access_key': 'abc123'}}, access_updates) if volume_client_version: (self._driver._volume_client.get_authorized_ids. assert_called_once_with(self._driver._share_path(self._share))) diff --git a/manila/tests/share/drivers/huawei/test_huawei_nas.py b/manila/tests/share/drivers/huawei/test_huawei_nas.py index 8a11429dd8..42155b3429 100644 --- a/manila/tests/share/drivers/huawei/test_huawei_nas.py +++ b/manila/tests/share/drivers/huawei/test_huawei_nas.py @@ -4284,16 +4284,20 @@ class HuaweiShareDriverTestCase(test.TestCase): 'access_rules_status': common_constants.STATUS_ACTIVE}, {'id': self.active_replica['id'], 'replica_state': common_constants.REPLICA_STATE_IN_SYNC, - 'access_rules_status': common_constants.STATUS_OUT_OF_SYNC}, + 'access_rules_status': + common_constants.SHARE_INSTANCE_RULES_SYNCING}, ] self.assertEqual(expected, result) @ddt.data({'mock_method': 'update_access', - 'new_access_status': common_constants.STATUS_OUT_OF_SYNC, - 'old_access_status': common_constants.STATUS_OUT_OF_SYNC}, + 'new_access_status': + common_constants.SHARE_INSTANCE_RULES_SYNCING, + 'old_access_status': + common_constants.SHARE_INSTANCE_RULES_SYNCING}, {'mock_method': 'clear_access', - 'new_access_status': common_constants.STATUS_OUT_OF_SYNC, + 'new_access_status': + common_constants.SHARE_INSTANCE_RULES_SYNCING, 'old_access_status': common_constants.STATUS_ACTIVE},) @ddt.unpack def test_promote_replica_with_access_update_error( @@ -4372,7 +4376,8 @@ class HuaweiShareDriverTestCase(test.TestCase): 'access_rules_status': common_constants.STATUS_ACTIVE}, {'id': self.active_replica['id'], 'replica_state': common_constants.STATUS_ERROR, - 'access_rules_status': common_constants.STATUS_OUT_OF_SYNC}, + 'access_rules_status': + common_constants.SHARE_INSTANCE_RULES_SYNCING}, ] self.assertEqual(expected, result) diff --git a/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py b/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py index fa49cd5030..ef951be600 100644 --- a/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py +++ b/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py @@ -2946,7 +2946,7 @@ class NetAppFileStorageLibraryTestCase(test.TestCase): actual_replica_2['replica_state']) self.assertEqual('fake_export_location', actual_replica_2['export_locations']) - self.assertEqual(constants.STATUS_OUT_OF_SYNC, + self.assertEqual(constants.SHARE_INSTANCE_RULES_SYNCING, actual_replica_2['access_rules_status']) def test_convert_destination_replica_to_independent_with_access_rules( @@ -2982,7 +2982,7 @@ class NetAppFileStorageLibraryTestCase(test.TestCase): replica['export_locations']) self.assertEqual(constants.REPLICA_STATE_ACTIVE, replica['replica_state']) - self.assertEqual(constants.STATUS_OUT_OF_SYNC, + self.assertEqual(constants.SHARE_INSTANCE_RULES_SYNCING, replica['access_rules_status']) def test_convert_destination_replica_to_independent_failed_access_rules( diff --git a/manila/tests/share/drivers/zfsonlinux/test_driver.py b/manila/tests/share/drivers/zfsonlinux/test_driver.py index 9ddbc24efa..446966709d 100644 --- a/manila/tests/share/drivers/zfsonlinux/test_driver.py +++ b/manila/tests/share/drivers/zfsonlinux/test_driver.py @@ -1738,13 +1738,15 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase): 'fake_context', replica_list, replica, access_rules) expected = [ - {'access_rules_status': zfs_driver.constants.STATUS_OUT_OF_SYNC, + {'access_rules_status': + zfs_driver.constants.SHARE_INSTANCE_RULES_SYNCING, 'id': 'fake_active_replica_id', 'replica_state': zfs_driver.constants.REPLICA_STATE_IN_SYNC}, {'access_rules_status': zfs_driver.constants.STATUS_ACTIVE, 'id': 'fake_first_replica_id', 'replica_state': zfs_driver.constants.REPLICA_STATE_ACTIVE}, - {'access_rules_status': zfs_driver.constants.STATUS_OUT_OF_SYNC, + {'access_rules_status': + zfs_driver.constants.SHARE_INSTANCE_RULES_SYNCING, 'id': 'fake_second_replica_id', 'replica_state': zfs_driver.constants.REPLICA_STATE_OUT_OF_SYNC}, ] @@ -1838,15 +1840,18 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase): 'fake_context', replica_list, replica, access_rules) expected = [ - {'access_rules_status': zfs_driver.constants.STATUS_OUT_OF_SYNC, + {'access_rules_status': + zfs_driver.constants.SHARE_INSTANCE_RULES_SYNCING, 'id': 'fake_active_replica_id', 'replica_state': zfs_driver.constants.REPLICA_STATE_OUT_OF_SYNC}, {'access_rules_status': zfs_driver.constants.STATUS_ACTIVE, 'id': 'fake_first_replica_id', 'replica_state': zfs_driver.constants.REPLICA_STATE_ACTIVE}, - {'access_rules_status': zfs_driver.constants.STATUS_OUT_OF_SYNC, + {'access_rules_status': + zfs_driver.constants.SHARE_INSTANCE_RULES_SYNCING, 'id': 'fake_second_replica_id'}, - {'access_rules_status': zfs_driver.constants.STATUS_OUT_OF_SYNC, + {'access_rules_status': + zfs_driver.constants.SHARE_INSTANCE_RULES_SYNCING, 'id': 'fake_third_replica_id', 'replica_state': zfs_driver.constants.REPLICA_STATE_OUT_OF_SYNC}, ] diff --git a/manila/tests/share/test_access.py b/manila/tests/share/test_access.py index b3c0f84ae2..7c9ed1a77e 100644 --- a/manila/tests/share/test_access.py +++ b/manila/tests/share/test_access.py @@ -13,6 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. +import itertools + import ddt import mock @@ -23,6 +25,199 @@ from manila import exception from manila.share import access from manila import test from manila.tests import db_utils +from manila import utils + + +class LockedOperationsTestCase(test.TestCase): + + class FakeAccessHelper(object): + + @access.locked_access_rules_operation + def some_access_rules_operation(self, context, share_instance_id=None): + pass + + def setUp(self): + super(self.__class__, self).setUp() + self.access_helper = self.FakeAccessHelper() + self.context = context.RequestContext('fake_user', 'fake_project') + self.lock_call = self.mock_object( + utils, 'synchronized', mock.Mock(return_value=lambda f: f)) + + def test_locked_access_rules_operation(self, **replica): + + self.access_helper.some_access_rules_operation( + self.context, share_instance_id='FAKE_INSTANCE_ID') + + self.lock_call.assert_called_once_with( + "locked_access_rules_operation_by_share_instance_FAKE_INSTANCE_ID", + external=True) + + +@ddt.ddt +class ShareInstanceAccessDatabaseMixinTestCase(test.TestCase): + + def setUp(self): + super(self.__class__, self).setUp() + self.driver = mock.Mock() + self.access_helper = access.ShareInstanceAccess(db, self.driver) + self.context = context.RequestContext('fake_user', 'fake_project') + self.mock_object( + utils, 'synchronized', mock.Mock(return_value=lambda f: f)) + + def test_get_and_update_access_rules_status_force_status(self): + share = db_utils.create_share( + access_rule_status=constants.STATUS_ACTIVE, + status=constants.STATUS_AVAILABLE) + share = db.share_get(self.context, share['id']) + self.assertEqual(constants.STATUS_ACTIVE, share['access_rules_status']) + + self.access_helper.get_and_update_share_instance_access_rules_status( + self.context, status=constants.SHARE_INSTANCE_RULES_SYNCING, + share_instance_id=share['instance']['id']) + + share = db.share_get(self.context, share['id']) + self.assertEqual(constants.SHARE_INSTANCE_RULES_SYNCING, + share['access_rules_status']) + + @ddt.data((constants.SHARE_INSTANCE_RULES_SYNCING, True), + (constants.STATUS_ERROR, False)) + @ddt.unpack + def test_get_and_update_access_rules_status_conditionally_change( + self, initial_status, change_allowed): + share = db_utils.create_share(access_rules_status=initial_status, + status=constants.STATUS_AVAILABLE) + share = db.share_get(self.context, share['id']) + self.assertEqual(initial_status, share['access_rules_status']) + + conditionally_change = { + constants.SHARE_INSTANCE_RULES_SYNCING: constants.STATUS_ACTIVE, + } + + updated_instance = ( + self.access_helper. + get_and_update_share_instance_access_rules_status( + self.context, conditionally_change=conditionally_change, + share_instance_id=share['instance']['id']) + ) + + share = db.share_get(self.context, share['id']) + if change_allowed: + self.assertEqual(constants.STATUS_ACTIVE, + share['access_rules_status']) + self.assertIsNotNone(updated_instance) + else: + self.assertEqual(initial_status, share['access_rules_status']) + self.assertIsNone(updated_instance) + + def test_get_and_update_all_access_rules_just_get(self): + share = db_utils.create_share(status=constants.STATUS_AVAILABLE) + rule_1 = db_utils.create_access(share_id=share['id']) + rule_2 = db_utils.create_access(share_id=share['id']) + self.mock_object(db, 'share_instance_access_update') + + rules = self.access_helper.get_and_update_share_instance_access_rules( + self.context, share_instance_id=share['instance']['id']) + + self.assertEqual(2, len(rules)) + rule_ids = [r['access_id'] for r in rules] + self.assertTrue(rule_1['id'] in rule_ids) + self.assertTrue(rule_2['id'] in rule_ids) + self.assertFalse(db.share_instance_access_update.called) + + @ddt.data( + ([constants.ACCESS_STATE_QUEUED_TO_APPLY], 2), + ([constants.ACCESS_STATE_QUEUED_TO_APPLY, + constants.STATUS_ACTIVE], 1), + ([constants.ACCESS_STATE_APPLYING], 2), + ([constants.ACCESS_STATE_APPLYING, constants.ACCESS_STATE_ERROR], 1), + ([constants.ACCESS_STATE_ACTIVE, constants.ACCESS_STATE_DENYING], 0)) + @ddt.unpack + def test_get_and_update_all_access_rules_updates_conditionally_changed( + self, statuses, changes_allowed): + share = db_utils.create_share(status=constants.STATUS_AVAILABLE) + db_utils.create_access(share_id=share['id'], state=statuses[0]) + db_utils.create_access(share_id=share['id'], state=statuses[-1]) + self.mock_object(db, 'share_instance_access_update', mock.Mock( + side_effect=db.share_instance_access_update)) + updates = { + 'access_key': 'renfrow2stars' + } + expected_updates = { + 'access_key': 'renfrow2stars', + 'state': constants.ACCESS_STATE_QUEUED_TO_DENY, + } + conditionally_change = { + constants.ACCESS_STATE_APPLYING: + constants.ACCESS_STATE_QUEUED_TO_DENY, + constants.ACCESS_STATE_QUEUED_TO_APPLY: + constants.ACCESS_STATE_QUEUED_TO_DENY, + } + + rules = self.access_helper.get_and_update_share_instance_access_rules( + self.context, share_instance_id=share['instance']['id'], + updates=updates, conditionally_change=conditionally_change) + + state_changed_rules = [ + r for r in rules if + r['state'] == constants.ACCESS_STATE_QUEUED_TO_DENY + ] + self.assertEqual(changes_allowed, len(state_changed_rules)) + self.assertEqual(2, db.share_instance_access_update.call_count) + db.share_instance_access_update.assert_has_calls([ + mock.call(self.context, mock.ANY, share['instance']['id'], + expected_updates), + ] * changes_allowed) + + def test_get_and_update_access_rule_just_get(self): + share = db_utils.create_share(status=constants.STATUS_AVAILABLE) + expected_rule = db_utils.create_access(share_id=share['id']) + self.mock_object(db, 'share_instance_access_update') + + actual_rule = ( + self.access_helper.get_and_update_share_instance_access_rule( + self.context, expected_rule['id'], + share_instance_id=share['instance']['id']) + ) + + self.assertEqual(expected_rule['id'], actual_rule['access_id']) + self.assertFalse(db.share_instance_access_update.called) + + @ddt.data(constants.ACCESS_STATE_APPLYING, + constants.ACCESS_STATE_DENYING, + constants.ACCESS_STATE_ACTIVE, + constants.ACCESS_STATE_QUEUED_TO_APPLY) + def test_get_and_update_access_rule_updates_conditionally_changed( + self, initial_state): + mock_debug_log = self.mock_object(access.LOG, 'debug') + share = db_utils.create_share(status=constants.STATUS_AVAILABLE) + rule = db_utils.create_access(share_id=share['id'], + state=initial_state) + self.mock_object(db, 'share_instance_access_update', mock.Mock( + side_effect=db.share_instance_access_update)) + updates = { + 'access_key': 'renfrow2stars' + } + conditionally_change = { + constants.ACCESS_STATE_APPLYING: + constants.ACCESS_STATE_QUEUED_TO_DENY, + constants.ACCESS_STATE_DENYING: + constants.ACCESS_STATE_QUEUED_TO_DENY, + } + + actual_rule = ( + self.access_helper.get_and_update_share_instance_access_rule( + self.context, rule['id'], updates=updates, + share_instance_id=share['instance']['id'], + conditionally_change=conditionally_change) + ) + self.assertEqual(rule['id'], actual_rule['access_id']) + if 'ing' in initial_state: + self.assertEqual(constants.ACCESS_STATE_QUEUED_TO_DENY, + actual_rule['state']) + self.assertFalse(mock_debug_log.called) + else: + self.assertEqual(initial_state, actual_rule['state']) + mock_debug_log.assert_called_once() @ddt.ddt @@ -31,282 +226,506 @@ class ShareInstanceAccessTestCase(test.TestCase): super(ShareInstanceAccessTestCase, self).setUp() self.driver = self.mock_class("manila.share.driver.ShareDriver", mock.Mock()) - self.share_access_helper = access.ShareInstanceAccess(db, self.driver) - self.context = context.get_admin_context() - self.share = db_utils.create_share() - 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') + self.access_helper = access.ShareInstanceAccess(db, self.driver) + self.context = context.RequestContext('fake_user', 'fake_project') - @ddt.data(True, False) - def test_update_access_rules_maintenance_mode(self, maintenance_mode): - existing_rules = [] - for i in range(2): - existing_rules.append( - db_utils.create_access( - id='fakeid%s' % i, - share_id=self.share['id'], - access_to='fakeip%s' % i, - )) - delete_rules = [existing_rules[0], ] - rules = [existing_rules[1], ] - 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) + @ddt.data(constants.ACCESS_STATE_APPLYING, constants.ACCESS_STATE_DENYING) + def test_update_access_rules_an_update_is_in_progress(self, initial_state): + share = db_utils.create_share(status=constants.STATUS_AVAILABLE) + share_instance = share['instance'] + db_utils.create_access(share_id=share['id'], state=initial_state) + mock_debug_log = self.mock_object(access.LOG, 'debug') + self.mock_object(self.access_helper, '_update_access_rules') + get_and_update_call = self.mock_object( + self.access_helper, 'get_and_update_share_instance_access_rules', + mock.Mock(side_effect=self.access_helper. + get_and_update_share_instance_access_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=existing_rules)) - self.mock_object(db, "share_instance_update_access_status", - 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", - mock.Mock(return_value=False)) - - self.share_access_helper.update_access_rules( - self.context, share_instance['id'], - delete_rules=delete_rules) - - self.driver.update_access.assert_called_once_with( - self.context, share_instance, rules, add_rules=[], - delete_rules=([] if maintenance_mode else delete_rules), - share_server=None) - self.share_access_helper._remove_access_rules.assert_called_once_with( - self.context, delete_rules, share_instance['id']) - self.share_access_helper._check_needs_refresh.assert_called_once_with( - self.context, rules, share_instance) - 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'])] - original_rules = [db_utils.create_access(share_id=self.share['id'])] - - self.mock_object(db, "share_instance_get", mock.Mock( - return_value=self.share_instance)) - self.mock_object(db, "share_access_get_all_for_share", - mock.Mock(return_value=original_rules)) - self.mock_object(db, "share_access_get_all_for_instance", - mock.Mock(return_value=original_rules)) - self.mock_object(db, "share_instance_update_access_status", - mock.Mock()) - self.mock_object(self.driver, "update_access", - mock.Mock(side_effect=NotImplementedError)) - self.mock_object(self.driver, "allow_access", - mock.Mock()) - self.mock_object(self.driver, "deny_access", - mock.Mock()) - - self.share_access_helper.update_access_rules(self.context, - self.share_instance['id'], - add_rules, delete_rules) - - self.driver.update_access.assert_called_with( - self.context, self.share_instance, original_rules, - add_rules=add_rules, delete_rules=[], share_server=None) - self.driver.allow_access.assert_called_with(self.context, - self.share_instance, - add_rules[0], - share_server=None) - self.assertFalse(self.driver.deny_access.called) - db.share_instance_update_access_status.assert_called_with( - self.context, self.share_instance['id'], constants.STATUS_ACTIVE) - - def test_update_access_rules_exception(self): - original_rules = [] - add_rules = [db_utils.create_access(share_id=self.share['id'])] - delete_rules = 'all' - - self.mock_object(db, "share_instance_get", mock.Mock( - return_value=self.share_instance)) - self.mock_object(db, "share_access_get_all_for_instance", - mock.Mock(return_value=original_rules)) - self.mock_object(db, "share_instance_update_access_status", - mock.Mock()) - self.mock_object(self.driver, "update_access", - mock.Mock(side_effect=exception.ManilaException)) - - self.assertRaises(exception.ManilaException, - self.share_access_helper.update_access_rules, - self.context, self.share_instance['id'], add_rules, - delete_rules) - - self.driver.update_access.assert_called_with( - self.context, self.share_instance, [], add_rules=add_rules, - delete_rules=original_rules, share_server=None) - - db.share_instance_update_access_status.assert_called_with( - self.context, self.share_instance['id'], constants.STATUS_ERROR) - - def test_update_access_rules_recursive_call(self): - share_instance = db_utils.create_share_instance( - access_rules_status=constants.STATUS_ACTIVE, - share_id=self.share['id']) - add_rules = [db_utils.create_access( - share_id=self.share['id'])] - original_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=original_rules)) - 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])) - - self.share_access_helper.update_access_rules(self.context, - share_instance['id'], - add_rules=add_rules) - - mock_update_access.assert_has_calls([ - mock.call(self.context, share_instance, original_rules, - add_rules=add_rules, delete_rules=[], share_server=None), - mock.call(self.context, share_instance, original_rules, - add_rules=[], delete_rules=[], share_server=None) - ]) - - @ddt.data(True, False) - def test_update_access_rules_migrating(self, read_only_support): - - def override_conf(conf_name): - if conf_name == 'migration_readonly_rules_support': - return read_only_support - - rules = [] - for i in range(2): - rules.append( - db_utils.create_access( - id='fakeid%s' % i, - share_id=self.share['id'], - access_to='fakeip%s' % i, - )) - driver_rules = [] if not read_only_support else rules - access_rules_status = constants.STATUS_OUT_OF_SYNC - share_instance = db_utils.create_share_instance( - id='fakeid', - status=constants.STATUS_MIGRATING, - share_id=self.share['id'], - access_rules_status=access_rules_status) - - 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=None)) - 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.mock_object(self.driver.configuration, 'safe_get', - mock.Mock(side_effect=override_conf)) - - self.share_access_helper.update_access_rules( + retval = self.access_helper.update_access_rules( self.context, share_instance['id']) - self.driver.update_access.assert_called_once_with( - self.context, share_instance, driver_rules, add_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) - db.share_instance_update_access_status.assert_called_with( - self.context, share_instance['id'], constants.STATUS_ACTIVE) + expected_filters = { + 'state': (constants.ACCESS_STATE_APPLYING, + constants.ACCESS_STATE_DENYING), + } + self.assertIsNone(retval) + mock_debug_log.assert_called_once() + get_and_update_call.assert_called_once_with( + self.context, filters=expected_filters, + share_instance_id=share_instance['id']) + self.assertFalse(self.access_helper._update_access_rules.called) + + def test_update_access_rules_nothing_to_update(self): + share = db_utils.create_share(status=constants.STATUS_AVAILABLE) + share_instance = share['instance'] + db_utils.create_access(share_id=share['id'], + state=constants.STATUS_ACTIVE) + mock_debug_log = self.mock_object(access.LOG, 'debug') + self.mock_object(self.access_helper, '_update_access_rules') + get_and_update_call = self.mock_object( + self.access_helper, 'get_and_update_share_instance_access_rules', + mock.Mock(side_effect=self.access_helper. + get_and_update_share_instance_access_rules)) + + retval = self.access_helper.update_access_rules( + self.context, share_instance['id']) + + expected_rule_filter_1 = { + 'state': (constants.ACCESS_STATE_APPLYING, + constants.ACCESS_STATE_DENYING), + } + expected_rule_filter_2 = { + 'state': (constants.ACCESS_STATE_QUEUED_TO_APPLY, + constants.ACCESS_STATE_QUEUED_TO_DENY), + } + expected_conditionally_change = { + constants.ACCESS_STATE_QUEUED_TO_APPLY: + constants.ACCESS_STATE_APPLYING, + constants.ACCESS_STATE_QUEUED_TO_DENY: + constants.ACCESS_STATE_DENYING, + } + self.assertIsNone(retval) + mock_debug_log.assert_called_once() + get_and_update_call.assert_has_calls( + [ + mock.call(self.context, filters=expected_rule_filter_1, + share_instance_id=share_instance['id']), + mock.call(self.context, filters=expected_rule_filter_2, + share_instance_id=share_instance['id'], + conditionally_change=expected_conditionally_change), + ]) + self.assertFalse(self.access_helper._update_access_rules.called) + + @ddt.data(True, False) + def test_update_access_rules_delete_all_rules(self, delete_all_rules): + share = db_utils.create_share(status=constants.STATUS_AVAILABLE) + share_instance = share['instance'] + db_utils.create_access( + share_id=share['id'], state=constants.STATUS_ACTIVE) + db_utils.create_access( + share_id=share['id'], state=constants.ACCESS_STATE_QUEUED_TO_APPLY) + db_utils.create_access( + share_id=share['id'], state=constants.ACCESS_STATE_QUEUED_TO_DENY) + mock_debug_log = self.mock_object(access.LOG, 'debug') + self.mock_object(self.access_helper, '_update_access_rules') + get_and_update_call = self.mock_object( + self.access_helper, 'get_and_update_share_instance_access_rules', + mock.Mock(side_effect=self.access_helper. + get_and_update_share_instance_access_rules)) + + retval = self.access_helper.update_access_rules( + self.context, share_instance['id'], + delete_all_rules=delete_all_rules) + + expected_rule_filter_1 = { + 'state': (constants.ACCESS_STATE_APPLYING, + constants.ACCESS_STATE_DENYING), + } + expected_rule_filter_2 = { + 'state': (constants.ACCESS_STATE_QUEUED_TO_APPLY, + constants.ACCESS_STATE_QUEUED_TO_DENY), + } + expected_conditionally_change = { + constants.ACCESS_STATE_QUEUED_TO_APPLY: + constants.ACCESS_STATE_APPLYING, + constants.ACCESS_STATE_QUEUED_TO_DENY: + constants.ACCESS_STATE_DENYING, + } + expected_get_and_update_calls = [] + if delete_all_rules: + deny_all_updates = { + 'state': constants.ACCESS_STATE_QUEUED_TO_DENY, + } + expected_get_and_update_calls = [ + mock.call(self.context, updates=deny_all_updates, + share_instance_id=share_instance['id']), + ] + expected_get_and_update_calls.extend([ + mock.call(self.context, filters=expected_rule_filter_1, + share_instance_id=share_instance['id']), + mock.call(self.context, filters=expected_rule_filter_2, + share_instance_id=share_instance['id'], + conditionally_change=expected_conditionally_change), + ]) + + self.assertIsNone(retval) + mock_debug_log.assert_called_once() + get_and_update_call.assert_has_calls(expected_get_and_update_calls) + self.access_helper._update_access_rules.assert_called_once_with( + self.context, share_instance['id'], share_server=None) + + @ddt.data(*itertools.product( + (True, False), (constants.ACCESS_STATE_ERROR, + constants.ACCESS_STATE_ACTIVE))) + @ddt.unpack + def test__update_access_rules_with_driver_updates( + self, driver_returns_updates, access_state): + expected_access_rules_status = ( + constants.STATUS_ACTIVE + if access_state == constants.ACCESS_STATE_ACTIVE + else constants.SHARE_INSTANCE_RULES_ERROR + ) + share = db_utils.create_share( + status=constants.STATUS_AVAILABLE, + access_rules_status=expected_access_rules_status) + share_instance_id = share['instance']['id'] + rule_1 = db_utils.create_access( + share_id=share['id'], state=access_state) + rule_1 = db.share_instance_access_get( + self.context, rule_1['id'], share_instance_id) + rule_2 = db_utils.create_access( + share_id=share['id'], state=constants.ACCESS_STATE_APPLYING) + rule_2 = db.share_instance_access_get( + self.context, rule_2['id'], share_instance_id) + rule_3 = db_utils.create_access( + share_id=share['id'], state=constants.ACCESS_STATE_DENYING) + rule_3 = db.share_instance_access_get( + self.context, rule_3['id'], share_instance_id) + if driver_returns_updates: + driver_rule_updates = { + rule_3['access_id']: {'access_key': 'alic3h4sAcc355'}, + rule_2['access_id']: {'state': access_state} + } + else: + driver_rule_updates = None + + shr_instance_access_rules_status_update_call = self.mock_object( + self.access_helper, + 'get_and_update_share_instance_access_rules_status', + mock.Mock(side_effect=self.access_helper. + get_and_update_share_instance_access_rules_status)) + all_access_rules_update_call = self.mock_object( + self.access_helper, 'get_and_update_share_instance_access_rules', + mock.Mock(side_effect=self.access_helper. + get_and_update_share_instance_access_rules)) + one_access_rule_update_call = self.mock_object( + self.access_helper, 'get_and_update_share_instance_access_rule', + mock.Mock(side_effect=self.access_helper. + get_and_update_share_instance_access_rule)) + + driver_call = self.mock_object( + self.access_helper.driver, 'update_access', + mock.Mock(return_value=driver_rule_updates)) + self.mock_object(self.access_helper, '_check_needs_refresh', + mock.Mock(return_value=False)) + + retval = self.access_helper._update_access_rules( + self.context, share_instance_id, share_server='fake_server') + + # Expected Values: + if access_state != constants.ACCESS_STATE_ERROR: + expected_rules_to_be_on_share = [r['id'] for r in (rule_1, rule_2)] + else: + expected_rules_to_be_on_share = [rule_2['id']] + + expected_filters_1 = { + 'state': (constants.ACCESS_STATE_APPLYING, + constants.ACCESS_STATE_ACTIVE, + constants.ACCESS_STATE_DENYING), + } + expected_filters_2 = {'state': constants.STATUS_ERROR} + expected_get_and_update_calls = [ + mock.call(self.context, filters=expected_filters_1, + share_instance_id=share_instance_id), + mock.call(self.context, filters=expected_filters_2, + share_instance_id=share_instance_id), + ] + expected_access_rules_status_change_cond1 = { + constants.STATUS_ACTIVE: constants.SHARE_INSTANCE_RULES_SYNCING, + } + if access_state == constants.SHARE_INSTANCE_RULES_ERROR: + expected_access_rules_status_change_cond2 = { + constants.SHARE_INSTANCE_RULES_SYNCING: + constants.SHARE_INSTANCE_RULES_ERROR, + } + else: + expected_access_rules_status_change_cond2 = { + constants.SHARE_INSTANCE_RULES_SYNCING: + constants.STATUS_ACTIVE, + constants.SHARE_INSTANCE_RULES_ERROR: + constants.STATUS_ACTIVE, + } + call_args = driver_call.call_args_list[0][0] + call_kwargs = driver_call.call_args_list[0][1] + access_rules_to_be_on_share = [r['id'] for r in call_args[2]] + + # Asserts + self.assertIsNone(retval) + self.assertEqual(share_instance_id, call_args[1]['id']) + self.assertEqual(sorted(expected_rules_to_be_on_share), + sorted(access_rules_to_be_on_share)) + self.assertEqual(1, len(call_kwargs['add_rules'])) + self.assertEqual(rule_2['id'], call_kwargs['add_rules'][0]['id']) + self.assertEqual(1, len(call_kwargs['delete_rules'])) + self.assertEqual(rule_3['id'], call_kwargs['delete_rules'][0]['id']) + self.assertEqual('fake_server', call_kwargs['share_server']) + shr_instance_access_rules_status_update_call.assert_has_calls([ + mock.call( + self.context, share_instance_id=share_instance_id, + conditionally_change=expected_access_rules_status_change_cond1 + ), + mock.call( + self.context, share_instance_id=share_instance_id, + conditionally_change=expected_access_rules_status_change_cond2 + ), + ]) + + if driver_returns_updates: + expected_conditional_state_updates = { + constants.ACCESS_STATE_APPLYING: access_state, + constants.ACCESS_STATE_DENYING: access_state, + constants.ACCESS_STATE_ACTIVE: access_state, + } + expected_access_rule_update_calls = [ + mock.call( + self.context, rule_3['access_id'], + updates={'access_key': 'alic3h4sAcc355'}, + share_instance_id=share_instance_id, + conditionally_change={}), + mock.call( + self.context, rule_2['access_id'], + updates=mock.ANY, share_instance_id=share_instance_id, + conditionally_change=expected_conditional_state_updates) + ] + one_access_rule_update_call.assert_has_calls( + expected_access_rule_update_calls, any_order=True) + else: + self.assertFalse(one_access_rule_update_call.called) + expected_conditionally_change = { + constants.ACCESS_STATE_APPLYING: constants.ACCESS_STATE_ACTIVE, + } + expected_get_and_update_calls.append( + mock.call(self.context, share_instance_id=share_instance_id, + conditionally_change=expected_conditionally_change)) + + all_access_rules_update_call.assert_has_calls( + expected_get_and_update_calls, any_order=True) + + share_instance = db.share_instance_get( + self.context, share_instance_id) + self.assertEqual(expected_access_rules_status, + share_instance['access_rules_status']) + + @ddt.data(True, False) + def test__update_access_rules_recursive_driver_exception(self, drv_exc): + other = access.ShareInstanceAccess(db, None) + share = db_utils.create_share( + status=constants.STATUS_AVAILABLE, + access_rules_status=constants.SHARE_INSTANCE_RULES_SYNCING) + share_instance_id = share['instance']['id'] + rule_4 = [] + get_and_update_count = [1] + drv_count = [1] + + def _get_and_update_side_effect(*args, **kwargs): + # The third call to this method needs to create a new access rule + mtd = other.get_and_update_share_instance_access_rules + if get_and_update_count[0] == 3: + rule_4.append( + db_utils.create_access( + state=constants.ACCESS_STATE_QUEUED_TO_APPLY, + share_id=share['id'])) + get_and_update_count[0] += 1 + return mtd(*args, **kwargs) + + def _driver_side_effect(*args, **kwargs): + if drv_exc and drv_count[0] == 2: + raise exception.ManilaException('fake') + drv_count[0] += 1 + + rule_kwargs = {'share_id': share['id'], 'access_level': 'rw'} + rule_1 = db_utils.create_access(state=constants.ACCESS_STATE_APPLYING, + **rule_kwargs) + rule_2 = db_utils.create_access(state=constants.ACCESS_STATE_ACTIVE, + **rule_kwargs) + rule_3 = db_utils.create_access(state=constants.ACCESS_STATE_DENYING, + **rule_kwargs) + + self.mock_object(self.access_helper, + 'get_and_update_share_instance_access_rules', + mock.Mock(side_effect=_get_and_update_side_effect)) + self.mock_object(self.access_helper.driver, 'update_access', + mock.Mock(side_effect=_driver_side_effect)) + + if drv_exc: + self.assertRaises(exception.ManilaException, + self.access_helper._update_access_rules, + self.context, share_instance_id) + else: + retval = self.access_helper._update_access_rules(self.context, + share_instance_id) + self.assertIsNone(retval) + + expected_filters_1 = { + 'state': (constants.ACCESS_STATE_APPLYING, + constants.ACCESS_STATE_ACTIVE, + constants.ACCESS_STATE_DENYING), + } + conditionally_change_2 = { + constants.ACCESS_STATE_APPLYING: constants.ACCESS_STATE_ACTIVE, + } + expected_filters_3 = { + 'state': (constants.ACCESS_STATE_QUEUED_TO_APPLY, + constants.ACCESS_STATE_QUEUED_TO_DENY), + } + expected_conditionally_change_3 = { + constants.ACCESS_STATE_QUEUED_TO_APPLY: + constants.ACCESS_STATE_APPLYING, + constants.ACCESS_STATE_QUEUED_TO_DENY: + constants.ACCESS_STATE_DENYING, + } + expected_conditionally_change_4 = { + constants.ACCESS_STATE_APPLYING: constants.ACCESS_STATE_ERROR, + constants.ACCESS_STATE_DENYING: constants.ACCESS_STATE_ERROR, + } + expected_get_and_update_calls = [ + mock.call(self.context, filters=expected_filters_1, + share_instance_id=share_instance_id), + mock.call(self.context, share_instance_id=share_instance_id, + conditionally_change=conditionally_change_2), + mock.call(self.context, filters=expected_filters_3, + share_instance_id=share_instance_id, + conditionally_change=expected_conditionally_change_3), + mock.call(self.context, filters=expected_filters_1, + share_instance_id=share_instance_id), + ] + + if drv_exc: + expected_get_and_update_calls.append( + mock.call( + self.context, share_instance_id=share_instance_id, + conditionally_change=expected_conditionally_change_4)) + else: + expected_get_and_update_calls.append( + mock.call(self.context, share_instance_id=share_instance_id, + conditionally_change=conditionally_change_2)) + + # Verify rule changes: + # 'denying' rule must not exist + self.assertRaises(exception.NotFound, + db.share_access_get, + self.context, rule_3['id']) + # 'applying' rule must be set to 'active' + rules_that_must_be_active = (rule_1, rule_2) + if not drv_exc: + rules_that_must_be_active += (rule_4[0], ) + for rule in rules_that_must_be_active: + rule = db.share_access_get(self.context, rule['id']) + self.assertEqual(constants.ACCESS_STATE_ACTIVE, + rule['state']) + # access_rules_status must be as expected + expected_access_rules_status = ( + constants.SHARE_INSTANCE_RULES_ERROR if drv_exc + else constants.STATUS_ACTIVE) + share_instance = db.share_instance_get(self.context, share_instance_id) + self.assertEqual( + expected_access_rules_status, + share_instance['access_rules_status']) + + @ddt.data(True, False) + def test__update_access_rules_for_migration_driver_supports_ro_rules( + self, driver_supports_ro_rules): + self.mock_object(self.driver.configuration, 'safe_get', mock.Mock( + return_value=driver_supports_ro_rules)) + share = db_utils.create_share( + status=constants.STATUS_MIGRATING, + access_rules_status=constants.STATUS_ACTIVE) + share_instance_id = share['instance']['id'] + rule_kwargs = {'share_id': share['id'], 'access_level': 'rw'} + rule_1 = db_utils.create_access( + state=constants.ACCESS_STATE_ACTIVE, **rule_kwargs) + rule_1 = db.share_instance_access_get( + self.context, rule_1['id'], share_instance_id) + rule_2 = db_utils.create_access( + state=constants.ACCESS_STATE_APPLYING, share_id=share['id'], + access_level='ro') + rule_2 = db.share_instance_access_get( + self.context, rule_2['id'], share_instance_id) + rule_3 = db_utils.create_access( + state=constants.ACCESS_STATE_DENYING, **rule_kwargs) + rule_3 = db.share_instance_access_get( + self.context, rule_3['id'], share_instance_id) + + driver_call = self.mock_object( + self.access_helper.driver, 'update_access', + mock.Mock(return_value=None)) + self.mock_object(self.access_helper, '_check_needs_refresh', + mock.Mock(return_value=False)) + + retval = self.access_helper._update_access_rules( + self.context, share_instance_id, share_server='fake_server') + + call_args = driver_call.call_args_list[0][0] + call_kwargs = driver_call.call_args_list[0][1] + access_rules_to_be_on_share = [r['id'] for r in call_args[2]] + access_levels = [r['access_level'] for r in call_args[2]] + expected_rules_to_be_on_share = ( + [rule_1['id'], rule_2['id']] + if driver_supports_ro_rules else [rule_2['id']] + ) + + self.assertIsNone(retval) + self.assertEqual(share_instance_id, call_args[1]['id']) + self.assertEqual(sorted(expected_rules_to_be_on_share), + sorted(access_rules_to_be_on_share)) + self.assertEqual(['ro'] * len(expected_rules_to_be_on_share), + access_levels) + self.assertEqual(0, len(call_kwargs['add_rules'])) + self.assertEqual(0, len(call_kwargs['delete_rules'])) + self.assertEqual('fake_server', call_kwargs['share_server']) + + @ddt.data(True, False) + def test__check_needs_refresh(self, expected_needs_refresh): + states = ( + [constants.ACCESS_STATE_QUEUED_TO_DENY, + constants.ACCESS_STATE_QUEUED_TO_APPLY] if expected_needs_refresh + else [constants.ACCESS_STATE_ACTIVE] + ) + share = db_utils.create_share( + status=constants.STATUS_AVAILABLE, + access_rules_status=constants.SHARE_INSTANCE_RULES_SYNCING) + share_instance_id = share['instance']['id'] + rule_kwargs = {'share_id': share['id'], 'access_level': 'rw'} + rule_1 = db_utils.create_access(state=states[0], **rule_kwargs) + db_utils.create_access(state=constants.ACCESS_STATE_ACTIVE, + **rule_kwargs) + db_utils.create_access(state=constants.ACCESS_STATE_DENYING, + **rule_kwargs) + rule_4 = db_utils.create_access(state=states[-1], **rule_kwargs) + + get_and_update_call = self.mock_object( + self.access_helper, 'get_and_update_share_instance_access_rules', + mock.Mock(side_effect=self.access_helper. + get_and_update_share_instance_access_rules)) + + needs_refresh = self.access_helper._check_needs_refresh( + self.context, share_instance_id) + + expected_filter = { + 'state': (constants.ACCESS_STATE_QUEUED_TO_APPLY, + constants.ACCESS_STATE_QUEUED_TO_DENY), + } + expected_conditionally_change = { + constants.ACCESS_STATE_QUEUED_TO_APPLY: + constants.ACCESS_STATE_APPLYING, + constants.ACCESS_STATE_QUEUED_TO_DENY: + constants.ACCESS_STATE_DENYING, + } + + self.assertEqual(expected_needs_refresh, needs_refresh) + get_and_update_call.assert_called_once_with( + self.context, filters=expected_filter, + share_instance_id=share_instance_id, + conditionally_change=expected_conditionally_change) + + rule_1 = db.share_instance_access_get( + self.context, rule_1['id'], share_instance_id) + rule_4 = db.share_instance_access_get( + self.context, rule_4['id'], share_instance_id) + + if expected_needs_refresh: + self.assertEqual(constants.ACCESS_STATE_DENYING, rule_1['state']) + self.assertEqual(constants.ACCESS_STATE_APPLYING, rule_4['state']) + else: + self.assertEqual(states[0], rule_1['state']) + self.assertEqual(states[-1], rule_4['state']) diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index 8dce301b41..0cf74d047d 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -1903,7 +1903,42 @@ class ShareAPITestCase(test.TestCase): share_api.policy.check_policy.assert_called_once_with( ctx, 'share_snapshot', 'get_all_snapshots') - @ddt.data(None, 'rw', 'ro') + def test_allow_access_rule_already_exists(self): + share = db_utils.create_share(status=constants.STATUS_AVAILABLE) + fake_access = db_utils.create_access(share_id=share['id']) + self.mock_object(self.api.db, 'share_access_create') + + self.assertRaises( + exception.ShareAccessExists, self.api.allow_access, + self.context, share, fake_access['access_type'], + fake_access['access_to'], fake_access['access_level']) + self.assertFalse(self.api.db.share_access_create.called) + + def test_allow_access_invalid_access_level(self): + share = db_utils.create_share(status=constants.STATUS_AVAILABLE) + self.mock_object(self.api.db, 'share_access_create') + + self.assertRaises( + exception.InvalidShareAccess, self.api.allow_access, + self.context, share, 'user', 'alice', access_level='execute') + self.assertFalse(self.api.db.share_access_create.called) + + @ddt.data({'host': None}, + {'status': constants.STATUS_ERROR_DELETING, + 'access_rules_status': constants.STATUS_ACTIVE}, + {'host': None, 'access_rules_status': constants.STATUS_ERROR}, + {'access_rules_status': constants.STATUS_ERROR}) + def test_allow_access_invalid_instance(self, params): + share = db_utils.create_share(host='fake') + db_utils.create_share_instance(share_id=share['id']) + db_utils.create_share_instance(share_id=share['id'], **params) + self.mock_object(self.api.db, 'share_access_create') + + self.assertRaises(exception.InvalidShare, self.api.allow_access, + self.context, share, 'ip', '10.0.0.1') + self.assertFalse(self.api.db.share_access_create.called) + + @ddt.data(*(constants.ACCESS_LEVELS + (None,))) def test_allow_access(self, level): share = db_utils.create_share(status=constants.STATUS_AVAILABLE) values = { @@ -1920,196 +1955,97 @@ class ShareAPITestCase(test.TestCase): 'deleted_at': 'fake_deleted_at', 'instance_mappings': ['foo', 'bar'], }) + self.mock_object(db_api, 'share_get', + mock.Mock(return_value=share)) self.mock_object(db_api, 'share_access_create', mock.Mock(return_value=fake_access)) self.mock_object(db_api, 'share_access_get', mock.Mock(return_value=fake_access)) + self.mock_object(db_api, 'share_access_get_all_by_type_and_access', + mock.Mock(return_value=[])) + self.mock_object(self.api, 'allow_access_to_instance') access = self.api.allow_access( self.context, share, fake_access['access_type'], fake_access['access_to'], level) self.assertEqual(fake_access, access) - self.share_rpcapi.allow_access.assert_called_once_with( - self.context, utils.IsAMatcher(models.ShareInstance), - fake_access) db_api.share_access_create.assert_called_once_with( self.context, values) - share_api.policy.check_policy.assert_called_with( - self.context, 'share', 'allow_access') + self.api.allow_access_to_instance.assert_called_once_with( + self.context, share.instance) - def test_allow_access_existent_access(self): - share = db_utils.create_share(status=constants.STATUS_AVAILABLE) - fake_access = db_utils.create_access(share_id=share['id']) + def test_allow_access_to_instance(self): + share = db_utils.create_share(host='fake') + rpc_method = self.mock_object(self.api.share_rpcapi, 'update_access') - self.assertRaises(exception.ShareAccessExists, self.api.allow_access, - self.context, share, fake_access['access_type'], - fake_access['access_to'], fake_access['access_level'] - ) + self.api.allow_access_to_instance(self.context, share.instance) - def test_allow_access_invalid_access_level(self): - share = db_utils.create_share(status=constants.STATUS_AVAILABLE) - self.assertRaises(exception.InvalidShareAccess, self.api.allow_access, - self.context, share, 'fakeacctype', 'fakeaccto', - 'ab') + rpc_method.assert_called_once_with(self.context, share.instance) - def test_allow_access_status_not_available(self): - share = db_utils.create_share(status=constants.STATUS_ERROR) - self.assertRaises(exception.InvalidShare, self.api.allow_access, - self.context, share, 'fakeacctype', 'fakeaccto') + @ddt.data({'host': None}, + {'status': constants.STATUS_ERROR_DELETING, + 'access_rules_status': constants.STATUS_ACTIVE}, + {'host': None, 'access_rules_status': constants.STATUS_ERROR}, + {'access_rules_status': constants.STATUS_ERROR}) + def test_deny_access_invalid_instance(self, params): + share = db_utils.create_share(host='fake') + db_utils.create_share_instance(share_id=share['id']) + db_utils.create_share_instance(share_id=share['id'], **params) + access_rule = db_utils.create_access(share_id=share['id']) + self.mock_object(self.api, 'deny_access_to_instance') - def test_allow_access_no_host(self): - share = db_utils.create_share(host=None) - self.assertRaises(exception.InvalidShare, self.api.allow_access, - self.context, share, 'fakeacctype', 'fakeaccto') + self.assertRaises(exception.InvalidShare, self.api.deny_access, + self.context, share, access_rule) + self.assertFalse(self.api.deny_access_to_instance.called) - @ddt.data(constants.STATUS_ACTIVE, constants.STATUS_UPDATING) - def test_allow_access_to_instance(self, status): + def test_deny_access(self): + share = db_utils.create_share( + host='fake', status=constants.STATUS_AVAILABLE, + access_rules_status=constants.STATUS_ACTIVE) + access_rule = db_utils.create_access(share_id=share['id']) + self.mock_object(self.api, 'deny_access_to_instance') + + retval = self.api.deny_access(self.context, share, access_rule) + + self.assertIsNone(retval) + self.api.deny_access_to_instance.assert_called_once_with( + self.context, share.instance, access_rule) + + def test_deny_access_to_instance(self): share = db_utils.create_share(host='fake') share_instance = db_utils.create_share_instance( - share_id=share['id'], access_rules_status=status, host='fake') + share_id=share['id'], host='fake') access = db_utils.create_access(share_id=share['id']) - rpc_method = self.mock_object(self.api.share_rpcapi, 'allow_access') - - self.api.allow_access_to_instance(self.context, share_instance, access) - - rpc_method.assert_called_once_with( - self.context, share_instance, access) - - def test_allow_access_to_instance_exception(self): - share = db_utils.create_share(host='fake') - access = db_utils.create_access(share_id=share['id']) - - share.instance['access_rules_status'] = constants.STATUS_ERROR - - self.assertRaises(exception.InvalidShareInstance, - self.api.allow_access_to_instance, self.context, - share.instance, access) - - def test_allow_access_to_instance_out_of_sync(self): - share = db_utils.create_share(host='fake') - access = db_utils.create_access(share_id=share['id']) - rpc_method = self.mock_object(self.api.share_rpcapi, 'allow_access') - - share.instance['access_rules_status'] = constants.STATUS_OUT_OF_SYNC - - self.api.allow_access_to_instance(self.context, share.instance, access) - rpc_method.assert_called_once_with( - self.context, share.instance, access) - - @ddt.data(constants.STATUS_ACTIVE, constants.STATUS_UPDATING, - constants.STATUS_UPDATING_MULTIPLE) - def test_deny_access_to_instance(self, status): - share = db_utils.create_share(host='fake') - share_instance = db_utils.create_share_instance( - share_id=share['id'], access_rules_status=status, host='fake') - access = db_utils.create_access(share_id=share['id']) - rpc_method = self.mock_object(self.api.share_rpcapi, 'deny_access') + rpc_method = self.mock_object(self.api.share_rpcapi, 'update_access') self.mock_object(db_api, 'share_instance_access_get', mock.Mock(return_value=access.instance_mappings[0])) - self.mock_object(db_api, 'share_instance_update_access_status') + mock_share_instance_rules_status_update = self.mock_object( + self.api.access_helper, + 'get_and_update_share_instance_access_rules_status') + mock_access_rule_state_update = self.mock_object( + self.api.access_helper, + 'get_and_update_share_instance_access_rule') self.api.deny_access_to_instance(self.context, share_instance, access) - if status == constants.STATUS_ACTIVE: - expected_new_status = constants.STATUS_OUT_OF_SYNC - else: - expected_new_status = constants.STATUS_UPDATING_MULTIPLE - - rpc_method.assert_called_once_with( - self.context, share_instance, access) - db_api.share_instance_update_access_status.assert_called_once_with( - self.context, - share_instance['id'], - expected_new_status - ) - - @ddt.data('allow_access_to_instance', 'deny_access_to_instance') - def test_allow_and_deny_access_to_instance_invalid_instance(self, method): - share = db_utils.create_share(host=None) - - self.assertRaises( - exception.InvalidShareInstance, - getattr(self.api, method), - self.context, share.instance, 'fake' - ) - - @mock.patch.object(db_api, 'share_get', mock.Mock()) - @mock.patch.object(share_api.API, 'deny_access_to_instance', mock.Mock()) - @mock.patch.object(db_api, 'share_instance_update_access_status', - mock.Mock()) - def test_deny_access_error(self): - share = db_utils.create_share(status=constants.STATUS_AVAILABLE) - db_api.share_get.return_value = share - access = db_utils.create_access(share_id=share['id']) - share_instance = share.instances[0] - - self.api.deny_access(self.context, share, access) - db_api.share_get.assert_called_once_with(self.context, share['id']) - share_api.policy.check_policy.assert_called_once_with( - self.context, 'share', 'deny_access') - share_api.API.deny_access_to_instance.assert_called_once_with( - self.context, share_instance, access) - - @mock.patch.object(db_api, 'share_get', mock.Mock()) - @mock.patch.object(db_api, 'share_access_delete', mock.Mock()) - def test_deny_access_error_no_share_instance_mapping(self): - share = db_utils.create_share(status=constants.STATUS_AVAILABLE) - db_api.share_get.return_value = share - access = db_utils.create_access(share_id=share['id']) - - self.api.deny_access(self.context, share, access) - - db_api.share_get.assert_called_once_with(self.context, share['id']) - self.assertTrue(share_api.policy.check_policy.called) - - @mock.patch.object(db_api, 'share_instance_update_access_status', - mock.Mock()) - def test_deny_access_active(self): - share = db_utils.create_share(status=constants.STATUS_AVAILABLE) - access = db_utils.create_access(share_id=share['id']) - self.api.deny_access(self.context, share, access) - db_api.share_instance_update_access_status.assert_called_once_with( - self.context, - share.instance['id'], - constants.STATUS_OUT_OF_SYNC - ) - share_api.policy.check_policy.assert_called_with( - self.context, 'share', 'deny_access') - self.share_rpcapi.deny_access.assert_called_once_with( - self.context, utils.IsAMatcher(models.ShareInstance), access) - - def test_deny_access_not_found(self): - share = db_utils.create_share(status=constants.STATUS_AVAILABLE) - access = db_utils.create_access(share_id=share['id']) - self.mock_object(db_api, 'share_instance_access_get', - mock.Mock(side_effect=[exception.NotFound('fake')])) - self.api.deny_access(self.context, share, access) - share_api.policy.check_policy.assert_called_with( - self.context, 'share', 'deny_access') - - def test_deny_access_status_not_available(self): - share = db_utils.create_share(status=constants.STATUS_ERROR) - self.assertRaises(exception.InvalidShare, self.api.deny_access, - self.context, share, 'fakeacc') - share_api.policy.check_policy.assert_called_once_with( - self.context, 'share', 'deny_access') - - def test_deny_access_no_host(self): - share = db_utils.create_share( - status=constants.STATUS_AVAILABLE, host=None) - self.assertRaises(exception.InvalidShare, self.api.deny_access, - self.context, share, 'fakeacc') - share_api.policy.check_policy.assert_called_once_with( - self.context, 'share', 'deny_access') + rpc_method.assert_called_once_with(self.context, share_instance) + mock_access_rule_state_update.assert_called_once_with( + self.context, access['id'], + updates={'state': constants.ACCESS_STATE_QUEUED_TO_DENY}, + share_instance_id=share_instance['id']) + expected_conditional_change = { + constants.STATUS_ACTIVE: constants.SHARE_INSTANCE_RULES_SYNCING, + } + mock_share_instance_rules_status_update.assert_called_once_with( + self.context, share_instance_id=share_instance['id'], + conditionally_change=expected_conditional_change) def test_access_get(self): with mock.patch.object(db_api, 'share_access_get', mock.Mock(return_value='fake')): rule = self.api.access_get(self.context, 'fakeid') self.assertEqual('fake', rule) - share_api.policy.check_policy.assert_called_once_with( - self.context, 'share', 'access_get') db_api.share_access_get.assert_called_once_with( self.context, 'fakeid') diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index 782decd671..ef12205e4d 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -32,7 +32,6 @@ from manila import db from manila.db.sqlalchemy import models from manila import exception from manila import quota -from manila.share import access as share_access from manila.share import api from manila.share import drivers_private_data from manila.share import manager @@ -193,8 +192,7 @@ class ShareManagerTestCase(test.TestCase): "delete_free_share_servers", "create_snapshot", "delete_snapshot", - "allow_access", - "deny_access", + "update_access", "_report_driver_status", "_execute_periodic_hook", "publish_service_capabilities", @@ -265,7 +263,8 @@ class ShareManagerTestCase(test.TestCase): display_name='fake_name_6').instance, ] - instances[4]['access_rules_status'] = constants.STATUS_OUT_OF_SYNC + instances[4]['access_rules_status'] = ( + constants.SHARE_INSTANCE_RULES_SYNCING) if not setup_access_rules: return instances @@ -639,7 +638,8 @@ class ShareManagerTestCase(test.TestCase): self.mock_object(db, 'share_instance_access_get', mock.Mock(return_value=fake_access_rules[0])) mock_share_replica_access_update = self.mock_object( - db, 'share_instance_update_access_status') + self.share_manager.access_helper, + 'get_and_update_share_instance_access_rules_status') self.mock_object(self.share_manager, '_get_share_server') driver_call = self.mock_object( @@ -650,9 +650,13 @@ class ShareManagerTestCase(test.TestCase): self.share_manager.create_share_replica, self.context, replica) mock_replica_update_call.assert_called_once_with( - mock.ANY, replica['id'], {'status': constants.STATUS_ERROR, - 'replica_state': constants.STATUS_ERROR}) - self.assertEqual(1, mock_share_replica_access_update.call_count) + utils.IsAMatcher(context.RequestContext), replica['id'], + {'status': constants.STATUS_ERROR, + 'replica_state': constants.STATUS_ERROR}) + mock_share_replica_access_update.assert_called_once_with( + utils.IsAMatcher(context.RequestContext), + share_instance_id=replica['id'], + status=constants.SHARE_INSTANCE_RULES_ERROR) self.assertFalse(mock_export_locs_update_call.called) self.assertTrue(mock_log_error.called) self.assertFalse(mock_log_info.called) @@ -662,7 +666,8 @@ class ShareManagerTestCase(test.TestCase): driver_retval = { 'export_locations': 'FAKE_EXPORT_LOC', } - replica = fake_replica(share_network='') + replica = fake_replica(share_network='', + access_rules_status=constants.STATUS_ACTIVE) replica_2 = fake_replica(id='fake2') fake_access_rules = [{'id': '1'}, {'id': '2'}] self.mock_object(db, 'share_replicas_get_available_active_replica', @@ -693,12 +698,15 @@ class ShareManagerTestCase(test.TestCase): self.mock_object(db, 'share_instance_access_get', mock.Mock(return_value=fake_access_rules[0])) mock_share_replica_access_update = self.mock_object( - db, 'share_instance_update_access_status') + self.share_manager.access_helper, + 'get_and_update_share_instance_access_rules_status') self.share_manager.create_share_replica(self.context, replica) self.assertFalse(mock_replica_update_call.called) - self.assertEqual(1, mock_share_replica_access_update.call_count) + mock_share_replica_access_update.assert_called_once_with( + utils.IsAMatcher(context.RequestContext), + share_instance_id=replica['id'], status=constants.STATUS_ACTIVE) self.assertFalse(mock_export_locs_update_call.called) self.assertTrue(mock_log_info.called) self.assertTrue(mock_log_warning.called) @@ -804,7 +812,8 @@ class ShareManagerTestCase(test.TestCase): self.mock_object(db, 'share_instance_access_get', mock.Mock(return_value=fake_access_rules[0])) mock_share_replica_access_update = self.mock_object( - db, 'share_instance_update_access_status') + self.share_manager.access_helper, + 'get_and_update_share_instance_access_rules_status') driver_call = self.mock_object( self.share_manager.driver, 'create_replica', mock.Mock(return_value=replica)) @@ -813,10 +822,13 @@ class ShareManagerTestCase(test.TestCase): self.share_manager.create_share_replica(self.context, replica) mock_replica_update_call.assert_called_once_with( - mock.ANY, replica['id'], + utils.IsAMatcher(context.RequestContext), replica['id'], {'status': constants.STATUS_AVAILABLE, 'replica_state': constants.REPLICA_STATE_IN_SYNC}) - self.assertEqual(1, mock_share_replica_access_update.call_count) + mock_share_replica_access_update.assert_called_once_with( + utils.IsAMatcher(context.RequestContext), + share_instance_id=replica['id'], + status=replica['access_rules_status']) self.assertTrue(mock_export_locs_update_call.called) self.assertTrue(mock_log_info.called) self.assertFalse(mock_log_warning.called) @@ -2292,7 +2304,7 @@ class ShareManagerTestCase(test.TestCase): smanager.driver.unmanage.assert_called_once_with(mock.ANY) smanager.access_helper.update_access_rules.assert_called_once_with( - mock.ANY, mock.ANY, delete_rules='all', share_server=None + mock.ANY, mock.ANY, delete_all_rules=True, share_server=None ) smanager.db.share_instance_delete.assert_called_once_with( mock.ANY, share_instance_id) @@ -2330,12 +2342,17 @@ class ShareManagerTestCase(test.TestCase): context.get_admin_context(), share_srv['id'], backend_details) share = db_utils.create_share(share_network_id=share_net['id'], share_server_id=share_srv['id']) + mock_access_helper_call = self.mock_object( + self.share_manager.access_helper, 'update_access_rules') self.share_manager.driver = mock.Mock() manager.CONF.delete_share_server_with_last_share = True self.share_manager.delete_share_instance(self.context, share.instance['id']) + mock_access_helper_call.assert_called_once_with( + utils.IsAMatcher(context.RequestContext), share.instance['id'], + delete_all_rules=True, share_server=mock.ANY) self.share_manager.driver.teardown_server.assert_called_once_with( server_details=backend_details, security_services=[jsonutils.loads( @@ -2353,19 +2370,24 @@ class ShareManagerTestCase(test.TestCase): ) share = db_utils.create_share(share_network_id=share_net['id'], share_server_id=share_srv['id']) - + share_srv = db.share_server_get(self.context, share_srv['id']) + mock_access_helper_call = self.mock_object( + self.share_manager.access_helper, 'update_access_rules') self.share_manager.driver = mock.Mock() if side_effect == 'update_access': - self.mock_object( - self.share_manager.access_helper, 'update_access_rules', - mock.Mock(side_effect=Exception('fake'))) + mock_access_helper_call.side_effect = exception.ManilaException if side_effect == 'delete_share': self.mock_object(self.share_manager.driver, 'delete_share', mock.Mock(side_effect=Exception('fake'))) self.mock_object(manager.LOG, 'error') manager.CONF.delete_share_server_with_last_share = True + self.share_manager.delete_share_instance( self.context, share.instance['id'], force=force) + + mock_access_helper_call.assert_called_once_with( + utils.IsAMatcher(context.RequestContext), share.instance['id'], + delete_all_rules=True, share_server=mock.ANY) self.share_manager.driver.teardown_server.assert_called_once_with( server_details=share_srv.get('backend_details'), security_services=[]) @@ -2379,11 +2401,21 @@ class ShareManagerTestCase(test.TestCase): ) share = db_utils.create_share(share_network_id=share_net['id'], share_server_id=share_srv['id']) + share_srv = db.share_server_get(self.context, share_srv['id']) manager.CONF.delete_share_server_with_last_share = False self.share_manager.driver = mock.Mock() + mock_access_helper_call = self.mock_object( + self.share_manager.access_helper, 'update_access_rules') + self.mock_object(db, 'share_server_get', + mock.Mock(return_value=share_srv)) + self.share_manager.delete_share_instance(self.context, share.instance['id']) + + mock_access_helper_call.assert_called_once_with( + utils.IsAMatcher(context.RequestContext), share.instance['id'], + delete_all_rules=True, share_server=share_srv) self.assertFalse(self.share_manager.driver.teardown_network.called) def test_delete_share_instance_not_last_on_server(self): @@ -2396,11 +2428,21 @@ class ShareManagerTestCase(test.TestCase): share_server_id=share_srv['id']) db_utils.create_share(share_network_id=share_net['id'], share_server_id=share_srv['id']) + share_srv = db.share_server_get(self.context, share_srv['id']) manager.CONF.delete_share_server_with_last_share = True self.share_manager.driver = mock.Mock() + self.mock_object(db, 'share_server_get', + mock.Mock(return_value=share_srv)) + mock_access_helper_call = self.mock_object( + self.share_manager.access_helper, 'update_access_rules') + self.share_manager.delete_share_instance(self.context, share.instance['id']) + + mock_access_helper_call.assert_called_once_with( + utils.IsAMatcher(context.RequestContext), share.instance['id'], + delete_all_rules=True, share_server=share_srv) self.assertFalse(self.share_manager.driver.teardown_network.called) @ddt.data('update_access', 'delete_share') @@ -2414,6 +2456,7 @@ class ShareManagerTestCase(test.TestCase): access = db_utils.create_access(share_id=share['id']) db_utils.create_share(share_network_id=share_net['id'], share_server_id=share_srv['id']) + share_srv = db.share_server_get(self.context, share_srv['id']) manager.CONF.delete_share_server_with_last_share = False @@ -2426,23 +2469,19 @@ class ShareManagerTestCase(test.TestCase): self.share_manager.driver = mock.Mock() self.share_manager.access_helper.driver = mock.Mock() if side_effect == 'update_access': - self.mock_object( - self.share_manager.access_helper.driver, 'update_access', + mock_access_helper_call = self.mock_object( + self.share_manager.access_helper, 'update_access_rules', 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_access_helper_call = self.mock_object( + self.share_manager.access_helper, 'update_access_rules', mock.Mock(return_value=None) ) self.mock_object( self.share_manager.driver, 'delete_share', mock.Mock(side_effect=exception.ShareResourceNotFound( share_id=share['id']))) - self.mock_object( - self.share_manager.access_helper, '_check_needs_refresh', - mock.Mock(return_value=False) - ) self.mock_object(manager.LOG, 'warning') @@ -2450,78 +2489,11 @@ class ShareManagerTestCase(test.TestCase): share.instance['id']) self.assertFalse(self.share_manager.driver.teardown_network.called) - (self.share_manager.access_helper.driver.update_access. - assert_called_once_with(utils.IsAMatcher( - context.RequestContext), share.instance, [], add_rules=[], - delete_rules=[access], share_server=share_srv)) - + mock_access_helper_call.assert_called_once_with( + utils.IsAMatcher(context.RequestContext), share.instance['id'], + delete_all_rules=True, share_server=share_srv) self.assertTrue(manager.LOG.warning.called) - def test_allow_deny_access(self): - """Test access rules to share can be created and deleted.""" - self.mock_object(share_access.LOG, 'info') - - share = db_utils.create_share() - share_id = share['id'] - share_instance = db_utils.create_share_instance( - share_id=share_id, - access_rules_status=constants.STATUS_OUT_OF_SYNC) - share_instance_id = share_instance['id'] - access = db_utils.create_access(share_id=share_id, - share_instance_id=share_instance_id) - access_id = access['id'] - - self.share_manager.allow_access(self.context, share_instance_id, - [access_id]) - self.assertEqual('active', db.share_instance_get( - self.context, share_instance_id).access_rules_status) - - share_access.LOG.info.assert_called_with(mock.ANY, - share_instance_id) - share_access.LOG.info.reset_mock() - - self.share_manager.deny_access(self.context, share_instance_id, - [access_id]) - - share_access.LOG.info.assert_called_with(mock.ANY, - share_instance_id) - share_access.LOG.info.reset_mock() - - def test_allow_deny_access_error(self): - """Test access rules to share can be created and deleted with error.""" - - def _fake_allow_access(self, *args, **kwargs): - raise exception.NotFound() - - def _fake_deny_access(self, *args, **kwargs): - raise exception.NotFound() - - self.mock_object(self.share_manager.access_helper.driver, - "allow_access", _fake_allow_access) - self.mock_object(self.share_manager.access_helper.driver, - "deny_access", _fake_deny_access) - - share = db_utils.create_share() - share_id = share['id'] - share_instance = db_utils.create_share_instance( - share_id=share_id, - access_rules_status=constants.STATUS_OUT_OF_SYNC) - share_instance_id = share_instance['id'] - access = db_utils.create_access(share_id=share_id, - share_instance_id=share_instance_id) - access_id = access['id'] - - def validate(method): - self.assertRaises(exception.ManilaException, method, self.context, - share_instance_id, [access_id]) - - inst = db.share_instance_get(self.context, share_instance_id) - self.assertEqual(constants.STATUS_ERROR, - inst['access_rules_status']) - - validate(self.share_manager.allow_access) - validate(self.share_manager.deny_access) - def test_setup_server(self): # Setup required test data share_server = { @@ -3794,8 +3766,8 @@ class ShareManagerTestCase(test.TestCase): mock.Mock(return_value=server)) self.mock_object(self.share_manager.db, 'share_instance_update', mock.Mock(return_value=server)) - self.mock_object(self.share_manager.db, - 'share_instance_update_access_status') + self.mock_object(self.share_manager.access_helper, + 'get_and_update_share_instance_access_rules') self.mock_object(self.share_manager.access_helper, 'update_access_rules') if exc is None: @@ -3828,18 +3800,15 @@ class ShareManagerTestCase(test.TestCase): self.share_manager.db.share_server_get.assert_called_once_with( utils.IsAMatcher(context.RequestContext), instance['share_server_id']) - (self.share_manager.db.share_instance_update_access_status. - assert_called_once_with(self.context, instance['id'], - constants.STATUS_OUT_OF_SYNC)) (self.share_manager.access_helper.update_access_rules. assert_called_once_with( self.context, instance['id'], share_server=server)) migration_api.ShareMigrationHelper.create_instance_and_wait.\ assert_called_once_with(share, 'fake_host', 'fake_net_id', 'fake_az_id', 'fake_type_id') - migration_api.ShareMigrationHelper.\ + (migration_api.ShareMigrationHelper. cleanup_access_rules.assert_called_once_with( - instance, server, self.share_manager.driver) + instance, server)) if exc is None: self.share_manager.db.share_instance_update.\ assert_called_once_with( @@ -3904,10 +3873,9 @@ class ShareManagerTestCase(test.TestCase): mock.Mock(return_value=dest_server)) self.mock_object(self.share_manager.driver, 'migration_start') self.mock_object(self.share_manager, '_migration_delete_instance') - self.mock_object(self.share_manager.db, - 'share_instance_update_access_status') self.mock_object(self.share_manager.access_helper, 'update_access_rules') + self.mock_object(utils, 'wait_for_access_update') # run if exc: @@ -3934,9 +3902,6 @@ class ShareManagerTestCase(test.TestCase): {'task_state': constants.TASK_STATE_MIGRATION_DRIVER_IN_PROGRESS}) ]) - (self.share_manager.db.share_instance_update_access_status. - assert_called_once_with(self.context, src_instance['id'], - constants.STATUS_OUT_OF_SYNC)) (self.share_manager.access_helper.update_access_rules. assert_called_once_with( self.context, src_instance['id'], share_server=src_server)) @@ -4020,6 +3985,7 @@ class ShareManagerTestCase(test.TestCase): self.mock_object(self.share_manager.driver, 'migration_check_compatibility', mock.Mock(return_value=compatibility)) + self.mock_object(utils, 'wait_for_access_update') # run self.assertRaises( @@ -4257,9 +4223,8 @@ class ShareManagerTestCase(test.TestCase): if status != 'other': migration_api.ShareMigrationHelper.cleanup_new_instance.\ assert_called_once_with(new_instance) - migration_api.ShareMigrationHelper.cleanup_access_rules.\ - assert_called_once_with(instance, server, - self.share_manager.driver) + (migration_api.ShareMigrationHelper.cleanup_access_rules. + assert_called_once_with(instance, server)) if status == constants.TASK_STATE_MIGRATION_CANCELLED: self.share_manager.db.share_instance_update.\ assert_called_once_with(self.context, instance['id'], @@ -4432,8 +4397,7 @@ class ShareManagerTestCase(test.TestCase): (migration_api.ShareMigrationHelper.cleanup_new_instance. assert_called_once_with(instance_2)) (migration_api.ShareMigrationHelper.cleanup_access_rules. - assert_called_once_with(instance_1, server_1, - self.share_manager.driver)) + assert_called_once_with(instance_1, server_1)) else: self.share_manager.driver.migration_cancel.assert_called_once_with( @@ -4466,19 +4430,19 @@ class ShareManagerTestCase(test.TestCase): def test__migration_delete_instance(self): instance = db_utils.create_share_instance(share_id='fake_id') - mapping_list = [{'id': 'mapping_id_1'}, {'id': 'mapping_id_2'}] rules = [{'id': 'rule_id_1'}, {'id': 'rule_id_2'}] # mocks self.mock_object(self.share_manager.db, 'share_instance_get', mock.Mock(return_value=instance)) self.mock_object(self.share_manager.db, 'share_instance_update') - self.mock_object( - self.share_manager.db, 'share_access_get_all_for_instance', + mock_get_access_rules_call = self.mock_object( + self.share_manager.access_helper, + 'get_and_update_share_instance_access_rules', mock.Mock(return_value=rules)) - self.mock_object( - self.share_manager.db, 'share_instance_access_get', mock.Mock( - side_effect=[mapping_list[0], mapping_list[1]])) + mock_delete_access_rules_call = self.mock_object( + self.share_manager.access_helper, + 'delete_share_instance_access_rules') self.mock_object(self.share_manager.db, 'share_instance_delete') self.mock_object(self.share_manager.db, 'share_instance_access_delete') self.mock_object(self.share_manager, '_check_delete_share_server') @@ -4493,16 +4457,10 @@ class ShareManagerTestCase(test.TestCase): self.share_manager.db.share_instance_update.assert_called_once_with( self.context, instance['id'], {'status': constants.STATUS_INACTIVE}) - (self.share_manager.db.share_access_get_all_for_instance. - assert_called_once_with(self.context, instance['id'])) - self.share_manager.db.share_instance_access_get.assert_has_calls([ - mock.call(self.context, 'rule_id_1', instance['id']), - mock.call(self.context, 'rule_id_2', instance['id']) - ]) - self.share_manager.db.share_instance_access_delete.assert_has_calls([ - mock.call(self.context, 'mapping_id_1'), - mock.call(self.context, 'mapping_id_2') - ]) + mock_get_access_rules_call.assert_called_once_with( + self.context, share_instance_id=instance['id']) + mock_delete_access_rules_call.assert_called_once_with( + self.context, rules, instance['id']) self.share_manager.db.share_instance_delete.assert_called_once_with( self.context, instance['id']) self.share_manager._check_delete_share_server.assert_called_once_with( @@ -5599,6 +5557,23 @@ class ShareManagerTestCase(test.TestCase): self.assertFalse(mock_db_update_call.called) self.assertFalse(mock_db_delete_call.called) + def test_update_access(self): + share_instance = fakes.fake_share_instance() + self.mock_object(self.share_manager, '_get_share_server', + mock.Mock(return_value='fake_share_server')) + self.mock_object(self.share_manager, '_get_share_instance', + mock.Mock(return_value=share_instance)) + access_rules_update_method = self.mock_object( + self.share_manager.access_helper, 'update_access_rules') + + retval = self.share_manager.update_access( + self.context, share_instance['id']) + + self.assertIsNone(retval) + access_rules_update_method.assert_called_once_with( + self.context, share_instance['id'], + share_server='fake_share_server') + @ddt.ddt class HookWrapperTestCase(test.TestCase): diff --git a/manila/tests/share/test_migration.py b/manila/tests/share/test_migration.py index 5ef08493da..7317f38a33 100644 --- a/manila/tests/share/test_migration.py +++ b/manila/tests/share/test_migration.py @@ -22,6 +22,7 @@ from manila.common import constants from manila import context from manila import db from manila import exception +from manila.share import access as access_helper from manila.share import api as share_api from manila.share import migration from manila import test @@ -39,9 +40,10 @@ class ShareMigrationHelperTestCase(test.TestCase): self.share_instance = db_utils.create_share_instance( share_id=self.share['id'], share_network_id='fake_network_id') + self.access_helper = access_helper.ShareInstanceAccess(db, None) self.context = context.get_admin_context() self.helper = migration.ShareMigrationHelper( - self.context, db, self.share) + self.context, db, self.share, self.access_helper) def test_delete_instance_and_wait(self): @@ -260,38 +262,31 @@ class ShareMigrationHelperTestCase(test.TestCase): server = db_utils.create_share_server(share_id=self.share['id']) # mocks - share_driver = mock.Mock() - self.mock_object(share_driver, 'update_access') - - self.mock_object(db, 'share_access_get_all_for_instance', - mock.Mock(return_value=[access])) + self.mock_object(self.access_helper, 'update_access_rules') + get_and_update_call = self.mock_object( + self.access_helper, 'get_and_update_share_instance_access_rules', + mock.Mock(return_value=[access])) # run - self.helper.revert_access_rules(share_instance, server, share_driver) + self.helper.revert_access_rules(share_instance, server) # asserts - db.share_access_get_all_for_instance.assert_called_once_with( - self.context, share_instance['id']) - share_driver.update_access.assert_called_once_with( - self.context, share_instance, [access], add_rules=[], - delete_rules=[], share_server=server) + get_and_update_call.assert_called_once_with( + self.context, share_instance_id=share_instance['id'], + updates={'state': constants.ACCESS_STATE_QUEUED_TO_APPLY}) + self.access_helper.update_access_rules.assert_called_once_with( + self.context, share_instance['id'], share_server=server) def test_apply_new_access_rules(self): new_share_instance = db_utils.create_share_instance( share_id=self.share['id'], status=constants.STATUS_AVAILABLE, access_rules_status='active') - - access = db_utils.create_access(share_id=self.share['id'], - access_to='fake_ip', - access_level='rw') + db_utils.create_access( + share_id=self.share['id'], access_to='fake_ip', access_level='rw') # mocks - self.mock_object(db, 'share_instance_get', - mock.Mock(return_value=new_share_instance)) self.mock_object(db, 'share_instance_access_copy') - self.mock_object(db, 'share_access_get_all_for_instance', - mock.Mock(return_value=[access])) self.mock_object(share_api.API, 'allow_access_to_instance') self.mock_object(utils, 'wait_for_access_update') @@ -299,14 +294,10 @@ class ShareMigrationHelperTestCase(test.TestCase): self.helper.apply_new_access_rules(new_share_instance) # asserts - db.share_instance_get.assert_called_once_with( - self.context, new_share_instance['id'], with_share_data=True) db.share_instance_access_copy(self.context, self.share['id'], new_share_instance['id']) - db.share_access_get_all_for_instance.assert_called_once_with( - self.context, new_share_instance['id']) share_api.API.allow_access_to_instance.assert_called_with( - self.context, new_share_instance, [access]) + self.context, new_share_instance) utils.wait_for_access_update.assert_called_with( self.context, db, new_share_instance, self.helper.migration_wait_access_rules_timeout) @@ -335,7 +326,6 @@ class ShareMigrationHelperTestCase(test.TestCase): # mocks server = db_utils.create_share_server() - share_driver = mock.Mock() self.mock_object(self.helper, 'revert_access_rules', mock.Mock(side_effect=exc)) self.mock_object(self.helper.db, 'share_instance_update') @@ -343,12 +333,11 @@ class ShareMigrationHelperTestCase(test.TestCase): self.mock_object(migration.LOG, 'warning') # run - self.helper.cleanup_access_rules(self.share_instance, server, - share_driver) + self.helper.cleanup_access_rules(self.share_instance, server) # asserts self.helper.revert_access_rules.assert_called_once_with( - self.share_instance, server, share_driver) + self.share_instance, server) self.helper.db.share_instance_update.assert_called_once_with( self.context, self.share_instance['id'], {'status': constants.STATUS_INACTIVE}) diff --git a/manila/tests/share/test_rpcapi.py b/manila/tests/share/test_rpcapi.py index 5322840747..e716806070 100644 --- a/manila/tests/share/test_rpcapi.py +++ b/manila/tests/share/test_rpcapi.py @@ -39,7 +39,6 @@ class ShareRpcAPITestCase(test.TestCase): availability_zone=CONF.storage_availability_zone, status=constants.STATUS_AVAILABLE ) - access = db_utils.create_access(share_id=share['id']) snapshot = db_utils.create_snapshot(share_id=share['id']) share_replica = db_utils.create_share_replica( id='fake_replica', @@ -55,7 +54,6 @@ class ShareRpcAPITestCase(test.TestCase): # doesn't know about those extra attributes to pull in self.fake_share['instance'] = jsonutils.to_primitive(share.instance) self.fake_share_replica = jsonutils.to_primitive(share_replica) - self.fake_access = jsonutils.to_primitive(access) self.fake_snapshot = jsonutils.to_primitive(snapshot) self.fake_share_server = jsonutils.to_primitive(share_server) self.fake_cg = jsonutils.to_primitive(cg) @@ -89,10 +87,6 @@ class ShareRpcAPITestCase(test.TestCase): snap = expected_msg['cgsnapshot'] del expected_msg['cgsnapshot'] expected_msg['cgsnapshot_id'] = snap['id'] - if 'access' in expected_msg: - access = expected_msg['access'] - del expected_msg['access'] - expected_msg['access_rules'] = [access['id']] if 'host' in expected_msg: del expected_msg['host'] if 'snapshot' in expected_msg: @@ -113,6 +107,9 @@ class ShareRpcAPITestCase(test.TestCase): if 'src_share_instance' in expected_msg: share_instance = expected_msg.pop('src_share_instance', None) expected_msg['src_instance_id'] = share_instance['id'] + if 'update_access' in expected_msg: + share_instance = expected_msg.pop('share_instance', None) + expected_msg['share_instance_id'] = share_instance['id'] if 'host' in kwargs: host = kwargs['host'] @@ -177,19 +174,11 @@ class ShareRpcAPITestCase(test.TestCase): share_instance=self.fake_share, force=False) - def test_allow_access(self): - self._test_share_api('allow_access', + def test_update_access(self): + self._test_share_api('update_access', rpc_method='cast', - version='1.7', - share_instance=self.fake_share, - access=self.fake_access) - - def test_deny_access(self): - self._test_share_api('deny_access', - rpc_method='cast', - version='1.7', - share_instance=self.fake_share, - access=self.fake_access) + version='1.14', + share_instance=self.fake_share) def test_create_snapshot(self): self._test_share_api('create_snapshot', diff --git a/manila/tests/test_utils.py b/manila/tests/test_utils.py index d500647d45..9fdf051084 100644 --- a/manila/tests/test_utils.py +++ b/manila/tests/test_utils.py @@ -691,8 +691,14 @@ class ShareMigrationHelperTestCase(test.TestCase): def test_wait_for_access_update(self): sid = 1 fake_share_instances = [ - {'id': sid, 'access_rules_status': constants.STATUS_OUT_OF_SYNC}, - {'id': sid, 'access_rules_status': constants.STATUS_ACTIVE}, + { + 'id': sid, + 'access_rules_status': constants.SHARE_INSTANCE_RULES_SYNCING, + }, + { + 'id': sid, + 'access_rules_status': constants.STATUS_ACTIVE, + }, ] self.mock_object(time, 'sleep') @@ -709,11 +715,17 @@ class ShareMigrationHelperTestCase(test.TestCase): @ddt.data( ( - {'id': '1', 'access_rules_status': constants.STATUS_ERROR}, + { + 'id': '1', + 'access_rules_status': constants.SHARE_INSTANCE_RULES_ERROR, + }, exception.ShareMigrationFailed ), ( - {'id': '1', 'access_rules_status': constants.STATUS_OUT_OF_SYNC}, + { + 'id': '1', + 'access_rules_status': constants.SHARE_INSTANCE_RULES_SYNCING, + }, exception.ShareMigrationFailed ), ) diff --git a/manila/utils.py b/manila/utils.py index d2cbcb6699..fe6b3f7414 100644 --- a/manila/utils.py +++ b/manila/utils.py @@ -550,7 +550,8 @@ def wait_for_access_update(context, db, share_instance, tries += 1 now = time.time() - if instance['access_rules_status'] == constants.STATUS_ERROR: + if (instance['access_rules_status'] == + constants.SHARE_INSTANCE_RULES_ERROR): msg = _("Failed to update access rules" " on share instance %s") % share_instance['id'] raise exception.ShareMigrationFailed(reason=msg) diff --git a/manila_tempest_tests/config.py b/manila_tempest_tests/config.py index 11631143d7..3756e63fee 100644 --- a/manila_tempest_tests/config.py +++ b/manila_tempest_tests/config.py @@ -30,7 +30,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.27", + default="2.28", help="The maximum api microversion is configured to be the " "value of the latest microversion supported by Manila."), cfg.StrOpt("region", diff --git a/releasenotes/notes/bug-1626249-reintroduce-per-share-instance-access-rule-state-7c08a91373b21557.yaml b/releasenotes/notes/bug-1626249-reintroduce-per-share-instance-access-rule-state-7c08a91373b21557.yaml new file mode 100644 index 0000000000..25960797be --- /dev/null +++ b/releasenotes/notes/bug-1626249-reintroduce-per-share-instance-access-rule-state-7c08a91373b21557.yaml @@ -0,0 +1,24 @@ +--- +features: + - New micro-states ('applying', 'denying'), appear in the 'state' field + of access rules list API. These transitional states signify the state of + an access rule while its application or denial is being processed + asynchronously. + - Access rules can be added regardless of the 'access_rules_status' + of the share or any of its replicas. +fixes: + - Fixed a bug with the share manager losing access rule updates when + multiple access rules are added to a given share simultaneously. + - Instead of all existing access rules transitioning to 'error' state + when some error occurs while applying or denying access rules to a given + share, only the rules that were in transitional statuses ('applying', + 'denying') during an update will transition to 'error' state. This + change is expected to aid in identifying any 'bad' rules that require a + resolution by the user. + - Share action APIs dealing with allowing and denying access to shares now + perform the policy check for authorization to invoke those APIs as a + preliminary step. + - As before, when a share is replicated (or being migrated), all replicas + (or migration instances) of the share must be in a valid state in order + to allow or deny access to the share (where such actions are otherwise + allowed). The check enforcing this in the API is fixed.