Add share state verification for API 'unmanage'

Shares with transitional states like 'creating' or 'deleting' should not be
allowed to be unmanaged.
For the moment "manage/unmanage" operation is allowed only for driver mode
without handling of share servers. So, API operations against shares created
on top of share servers should be restricted on API level.

Change-Id: I73b554448eadcb96ef00f44535014e14dea91472
Closes-Bug: #1434642
Closes-Bug: #1434511

manila/api/contrib/share_unmanage.py

@@ -19,7 +19,9 @@ from webob import exc
 
 from manila.api import extensions
 from manila.api.openstack import wsgi
+from manila.common import constants
 from manila import exception
+from manila.i18n import _
 from manila.i18n import _LI
 from manila import share
 
@@ -43,6 +45,18 @@ class ShareUnmanageController(wsgi.Controller):
 
         try:
             share = self.share_api.get(context, id)
+            if share.get('share_server_id'):
+                msg = _("Operation 'unmanage' is not supported for shares "
+                        "that are created on top of share servers "
+                        "(created with share-networks).")
+                raise exc.HTTPForbidden(explanation=msg)
+            # NOTE(vponomaryov): use 'upper' translation because share
+            # statuses not always used from common place yet.
+            elif share['status'].upper() in constants.TRANSITIONAL_STATUSES:
+                msg = _("Share with transitional state can not be unmanaged. "
+                        "Share '%(s_id)s' is in '%(state)s' state.") % dict(
+                            state=share['status'], s_id=share['id'])
+                raise exc.HTTPForbidden(explanation=msg)
             self.share_api.unmanage(context, share)
         except exception.NotFound as e:
             raise exc.HTTPNotFound(explanation=six.text_type(e))
@@ -66,4 +80,4 @@ class Share_unmanage(extensions.ExtensionDescriptor):
         res = extensions.ResourceExtension(Share_unmanage.alias,
                                            controller,
                                            member_actions={"unmanage": "POST"})
-        return [res]
+        return [res]

manila/common/constants.py

@@ -18,6 +18,8 @@ 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_ACTIVATING = 'ACTIVATING'
@@ -28,6 +30,12 @@ STATUS_UNMANAGING = 'UNMANAGE_STARTING'
 STATUS_UNMANAGE_ERROR = 'UNMANAGE_ERROR'
 STATUS_UNMANAGED = 'UNMANAGED'
 
+TRANSITIONAL_STATUSES = (
+    STATUS_CREATING, STATUS_DELETING,
+    STATUS_ACTIVATING, STATUS_DEACTIVATING,
+    STATUS_MANAGING, STATUS_UNMANAGING,
+)
+
 SUPPORTED_SHARE_PROTOCOLS = (
     'NFS', 'CIFS', 'GLUSTERFS', 'HDFS')
 
@@ -71,4 +79,4 @@ ACCESS_LEVELS = (
 
 class ExtraSpecs(object):
     DRIVER_HANDLES_SHARE_SERVERS = "driver_handles_share_servers"
-    REQUIRED = (DRIVER_HANDLES_SHARE_SERVERS, )
+    REQUIRED = (DRIVER_HANDLES_SHARE_SERVERS, )

manila/tests/api/contrib/test_share_unmanage.py

@@ -18,6 +18,7 @@ import mock
 import webob
 
 from manila.api.contrib import share_unmanage
+from manila.common import constants
 from manila import exception
 from manila.share import api as share_api
 from manila import test
@@ -46,13 +47,41 @@ class ShareUnmanageTest(test.TestCase):
         )
 
     def test_unmanage_share(self):
-        self.mock_object(share_api.API, 'get', mock.Mock(return_value={}))
+        share = dict(status=constants.STATUS_AVAILABLE, id='foo_id')
+        self.mock_object(share_api.API, 'get', mock.Mock(return_value=share))
         self.mock_object(share_api.API, 'unmanage', mock.Mock())
 
         actual_result = self.controller.unmanage(self.request, self.share_id)
 
         self.assertEqual(202, actual_result.status_int)
 
+    def test_unmanage_share_based_on_share_server(self):
+        share = dict(share_server_id='foo_id', id='bar_id')
+        self.mock_object(
+            self.controller.share_api, 'get',
+            mock.Mock(return_value=share))
+
+        self.assertRaises(
+            webob.exc.HTTPForbidden,
+            self.controller.unmanage, self.request, share['id'])
+
+        self.controller.share_api.get.assert_called_once_with(
+            self.request.environ['manila.context'], share['id'])
+
+    @ddt.data(*constants.TRANSITIONAL_STATUSES)
+    def test_unmanage_share_with_transitional_state(self, share_status):
+        share = dict(status=share_status, id='foo_id')
+        self.mock_object(
+            self.controller.share_api, 'get',
+            mock.Mock(return_value=share))
+
+        self.assertRaises(
+            webob.exc.HTTPForbidden,
+            self.controller.unmanage, self.request, share['id'])
+
+        self.controller.share_api.get.assert_called_once_with(
+            self.request.environ['manila.context'], share['id'])
+
     def test_unmanage_share_not_found(self):
         self.mock_object(share_api.API, 'get', mock.Mock(
             side_effect=exception.NotFound))
@@ -65,7 +94,8 @@ class ShareUnmanageTest(test.TestCase):
     @ddt.data(exception.InvalidShare(reason="fake"),
               exception.PolicyNotAuthorized(action="fake"),)
     def test_unmanage_share_invalid(self, side_effect):
-        self.mock_object(share_api.API, 'get', mock.Mock(return_value={}))
+        share = dict(status=constants.STATUS_AVAILABLE, id='foo_id')
+        self.mock_object(share_api.API, 'get', mock.Mock(return_value=share))
         self.mock_object(share_api.API, 'unmanage', mock.Mock(
             side_effect=side_effect))